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

Re: [libvirt] [PATCH] introduce VIR_CLOSE to be used rather than close()



 On 10/15/2010 01:01 PM, Stefan Berger wrote:
 On 10/15/2010 12:54 PM, Matthias Bolte wrote:
2010/10/15 Stefan Berger<stefanb linux vnet ibm com>:
Since bugs due to double-closed filedescriptors are difficult to track down in a multi-threaded system, I am introducing the VIR_CLOSE(fd) macro to help
avoid mistakes here.

There are lots of places where close() is being used. In this patch I am
only cleaning up usage of close() in src/conf where the problems were.

I also dare to declare close() as being deprecated in libvirt code base
(HACKING). :-)

Signed-off-by: Stefan Berger<stefanb us ibm com>

Index: libvirt-acl/src/conf/domain_conf.c
===================================================================
--- libvirt-acl.orig/src/conf/domain_conf.c
+++ libvirt-acl/src/conf/domain_conf.c
@@ -46,6 +46,7 @@
  #include "nwfilter_conf.h"
  #include "ignore-value.h"
  #include "storage_file.h"
+#include "files.h"

  #define VIR_FROM_THIS VIR_FROM_DOMAIN

@@ -6798,17 +6799,18 @@ int virDomainSaveXML(const char *configD
         goto cleanup;
     }

-    if (close(fd)<  0) {
+    if (VIR_CLOSE(fd)<  0) {
         virReportSystemError(errno,
                              _("cannot save config file '%s'"),
                              configFile);
-        goto cleanup;
+        goto cleanup_free;
     }

     ret = 0;
  cleanup:
-    if (fd != -1)
-        close(fd);
+    VIR_CLOSE(fd);
+
+ cleanup_free:
     VIR_FREE(configFile);
     return ret;
  }
Why did you add a new label here? You build VIR_CLOSE in a way that
allows safe double invocation. Therefore, this is just fine, isn't it:

I had it without this new label and then changed it since I think it's still good practice to not try to invoke the function twice, even though nothing can go wrong.

    Stefan

VIR_FREE() is commonly used in the way that Matthias describes, so I think it would be consistent to use VIR_CLOSE in this way as well.

Invoking it unconditionally simplifies the code, and eliminates possible confusion when someone adds new code and isn't sure which label they should goto on failure (or maybe they think they are sure, but make the wrong assumption, thus leading to a leaked fd as a result of trying to eliminate possible double-closed fd's ;-)


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