[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]