[libvirt] [PATCH] deprecate fclose() and introduce VIR_{FORCE_}FCLOSE()
Stefan Berger
stefanb at linux.vnet.ibm.com
Fri Nov 12 17:47:04 UTC 2010
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
More information about the libvir-list
mailing list