[libvirt] [PATCH] deprecate fclose() and introduce VIR_{FORCE_}FCLOSE()

[Stefan Berger]

On 11/12/2010 11:58 AM, Eric Blake wrote:
> On 11/12/2010 09:38 AM, Stefan Berger wrote:
>> Similarly to deprecating close(), I am now deprecating fclose() and
>> introduce VIR_FORCE_FCLOSE() and VIR_FCLOSE().
>>
>> Most of the files are opened in read-only mode, so usage of
>> VIR_FORCE_CLOSE() seemed appropriate. Others that are opened in write
>> mode already had the fclose()<  0 check and I converted those to
>> VIR_FCLOSE()<  0.
>>
>> I did not find occurences of possible double-closed files on the way.
>>
>> Signed-off-by: Stefan Berger<stefanb at us.ibm.com>
>>
>> +
>> +int virFClose(FILE **file, bool preserve_errno)
> I would have named it virFclose, but it doesn't matter (the _only_ place
> that cares is the macro definition in files.h :)
I can rename that.
>> @@ -909,7 +909,7 @@ openvzSetDefinedUUID(int vpsid, unsigned
>>           /* Record failure if fprintf or fclose fails,
>>              and be careful always to close the stream.  */
>>           if ((fprintf(fp, "\n#UUID: %s\n", uuidstr)<  0)
>> -            + (fclose(fp) == EOF))
>> +            + (VIR_FCLOSE(fp) == EOF))
>>               goto cleanup;
> Ouch!  This is a pre-existing bug.  C does NOT guarantee order of
> operations across + (unlike Java).  Therefore, you MUST split this into
> a sequence point, in order to avoid risking fprintf(NULL) because
> close(fp) (pre-patch, or VIR_FCLOSE post-patch) got scheduled first by
> the compiler.

> Thankfully, || is a sequence point, and the only other trick is to
> realize that it is safe to blindly call VIR_FORCE_FCLOSE(fp) in the
> cleanup: label if the fprintf failed, and harmless if the VIR_FCLOSE was
> reached:
>
> if ((fprintf(fp, "\n#UUID: %s\n", uuidstr)<  0) ||
>      (VIR_FCLOSE(fp) == EOF))
>      goto cleanup;
> ...
> cleanup:
> VIR_FORCE_FCLOSE(fp);
>
Will fix it.
>> +++ libvirt-acl/src/storage/storage_backend.c
>> @@ -1458,10 +1458,8 @@ virStorageBackendRunProgRegex(virStorage
>>
>>       VIR_FREE(reg);
>>
>> -    if (list)
>> -        fclose(list);
>> -    else
>> -        VIR_FORCE_CLOSE(fd);
>> +    VIR_FORCE_FCLOSE(list);
>> +    VIR_FORCE_CLOSE(fd);
> You just introduced a double close.  list was created via fdopen(fd),
:-( In that case, what about wrapping fdopen with FDOPEN and set fd to 
-1 if the function succeeds, i.e., returns != NULL?
> which effectively consumes fd.  Maybe we need to add VIR_FDOPEN which
> auto-sets an fdopen'd fd to -1 on success?  Maybe not, but then you need
> to adjust the fdopen() call site to invalidate fd so we don't re-close it.
>
>> +++ libvirt-acl/src/storage/storage_backend_iscsi.c
>> @@ -235,11 +235,8 @@ out:
>>       }
>>
>>       VIR_FREE(line);
>> -    if (fp != NULL) {
>> -        fclose(fp);
>> -    } else {
>> -        VIR_FORCE_CLOSE(fd);
>> -    }
>> +    VIR_FORCE_FCLOSE(fp);
>> +    VIR_FORCE_CLOSE(fd);
> Same double-close bug.
>
OK.
>> @@ -219,7 +220,7 @@ xenUnifiedProbe (void)
>>       FILE *fh;
>>
>>       if (fh = fopen("/dev/xen/domcaps", "r")) {
>> -        fclose(fh);
>> +        VIR_FORCE_FCLOSE(fh);
> This is wasteful.  If we aren't going to use the FILE*, why not go with
> the much-faster open("/dev/xen/domcaps", O_RDONLY), VIR_FORCE_CLOSE
> sequence, to avoid malloc() overhead in stdio?
>
Ok, can change that also.
>> Index: libvirt-acl/docs/hacking.html.in
>> ===================================================================
>> --- libvirt-acl.orig/docs/hacking.html.in
>> +++ libvirt-acl/docs/hacking.html.in
>> @@ -392,9 +392,10 @@
>>       <h2><a name="file_handling">File handling</a></h2>
>>
>>       <p>
>> -      Use of the close() API is deprecated in libvirt code base to help
>> -      avoiding double-closing of a file descriptor. Instead of this API,
>> -      use the macro from files.h
>> +      Usage of the close() and fclose() APIs is deprecated in libvirt code base to help
> How about we mark these up:<code>close()</code>  and<code>fclose()</code>
>
Will do.

     Stefan