Re: [libvirt] [PATCH] deprecate fclose() and introduce VIR_{FORCE_}FCLOSE()
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)