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@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 :)
@@ -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);
+++ 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), 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.
@@ -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?
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> -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org