Introduce VIR_CLOSE to be used rather than close()

Since bugs due to double-closed file descriptors 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).

diff --git a/HACKING b/HACKING
index e22d73c71e433b5acd2c9e7c6219fce22a777fe1..a9a9b49924d6ac7a6524c6a5774edbb29e66d91f 100644 (file)
--- a/HACKING
+++ b/HACKING
@@ -318,6 +318,23 @@ routines, use the macros from memory.h
        VIR_FREE(domain);
 
 
+File handling
+=============
+
+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
+
+   - eg close a file descriptor
+
+       if (VIR_CLOSE(fd) < 0) {
+           virReportSystemError(errno, _("failed to close file"));
+       }
+
+   - eg close a file descriptor in an error path, without losing the previous
+     errno value
+
+       VIR_FORCE_CLOSE(fd);
 
 String comparisons
 ==================

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index deab530abc9abacd89525d4c012fcaece4963f7e..bd8b443a941e0a94b707cfca3c98f595c92b417b 100644 (file)
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -389,7 +389,30 @@
 </pre></li>
     </ul>
 
+    <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
+    </p>
+
+    <ul>
+      <li><p>eg close a file descriptor</p>
+
+<pre>
+       if (VIR_CLOSE(fd) &lt; 0) {
+           virReportSystemError(errno, _("failed to close file"));
+       }
+</pre></li>
+
+      <li><p>eg close a file descriptor in an error path, without losing
+             the previous errno value</p>
+
+<pre>
+       VIR_FORCE_CLOSE(fd);
+</pre></li>
+    </ul>
 
     <h2><a name="string_comparision">String comparisons</a></h2>
 

diff --git a/src/Makefile.am b/src/Makefile.am
index 9bc42872c34ff4d25e77fc55f029fff3f217f627..e386bfafaa9c066dd871250ec6289bc3abd9eca6 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -82,7 +82,8 @@ UTIL_SOURCES =                                                        \
                util/uuid.c util/uuid.h                         \
                util/util.c util/util.h                         \
                util/xml.c util/xml.h                           \
-               util/virterror.c util/virterror_internal.h
+               util/virterror.c util/virterror_internal.h      \
+               util/files.c util/files.h
 
 EXTRA_DIST += util/threads-pthread.c util/threads-win32.c
 

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ad8085f79f4990652fcabd1698dba33aa8f9ab3b..78d7a6ad5b05f2a56bef2ece63eb40b8edc7f3f1 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/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,7 +6799,7 @@ int virDomainSaveXML(const char *configDir,
         goto cleanup;
     }
 
-    if (close(fd) < 0) {
+    if (VIR_CLOSE(fd) < 0) {
         virReportSystemError(errno,
                              _("cannot save config file '%s'"),
                              configFile);
@@ -6807,8 +6808,8 @@ int virDomainSaveXML(const char *configDir,
 
     ret = 0;
  cleanup:
-    if (fd != -1)
-        close(fd);
+    VIR_FORCE_CLOSE(fd);
+
     VIR_FREE(configFile);
     return ret;
 }
@@ -7765,10 +7766,14 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
         }
 
         if (virStorageFileGetMetadataFromFD(path, fd, format, &meta) < 0) {
-            close(fd);
+            VIR_FORCE_CLOSE(fd);
             goto cleanup;
         }
-        close(fd);
+
+        if (VIR_CLOSE(fd) < 0)
+            virReportSystemError(errno,
+                                 _("could not close file %s"),
+                                 path);
 
         if (virHashAddEntry(paths, path, (void*)0x1) < 0) {
             virReportOOMError();

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 4c0248c5dfbaef52fd255210df02b9c735e60f90..ddef7909d63796c60174b17e56bc36022679cae8 100644 (file)
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -43,6 +43,7 @@
 #include "util.h"
 #include "buf.h"
 #include "c-ctype.h"
+#include "files.h"
 
 #define MAX_BRIDGE_ID 256
 #define VIR_FROM_THIS VIR_FROM_NETWORK
@@ -687,7 +688,7 @@ int virNetworkSaveXML(const char *configDir,
         goto cleanup;
     }
 
-    if (close(fd) < 0) {
+    if (VIR_CLOSE(fd) < 0) {
         virReportSystemError(errno,
                              _("cannot save config file '%s'"),
                              configFile);
@@ -697,8 +698,7 @@ int virNetworkSaveXML(const char *configDir,
     ret = 0;
 
  cleanup:
-    if (fd != -1)
-        close(fd);
+    VIR_FORCE_CLOSE(fd);
 
     VIR_FREE(configFile);
 

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index a553b516485d6b1746a8efa8f07e3b6217833796..40fbf5e2f47a2364d73251337e2f013d3639bbe3 100644 (file)
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -45,6 +45,7 @@
 #include "nwfilter_conf.h"
 #include "domain_conf.h"
 #include "c-ctype.h"
+#include "files.h"
 
 
 #define VIR_FROM_THIS VIR_FROM_NWFILTER
@@ -2193,7 +2194,7 @@ int virNWFilterSaveXML(const char *configDir,
         goto cleanup;
     }
 
-    if (close(fd) < 0) {
+    if (VIR_CLOSE(fd) < 0) {
         virReportSystemError(errno,
                              _("cannot save config file '%s'"),
                              configFile);
@@ -2203,9 +2204,7 @@ int virNWFilterSaveXML(const char *configDir,
     ret = 0;
 
  cleanup:
-    if (fd != -1)
-        close(fd);
-
+    VIR_FORCE_CLOSE(fd);
     VIR_FREE(configFile);
 
     return ret;
@@ -2604,7 +2603,7 @@ virNWFilterPoolObjSaveDef(virNWFilterDriverStatePtr driver,
         goto cleanup;
     }
 
-    if (close(fd) < 0) {
+    if (VIR_CLOSE(fd) < 0) {
         virReportSystemError(errno,
                              _("cannot save config file %s"),
                              pool->configFile);
@@ -2614,8 +2613,7 @@ virNWFilterPoolObjSaveDef(virNWFilterDriverStatePtr driver,
     ret = 0;
 
  cleanup:
-    if (fd != -1)
-        close(fd);
+    VIR_FORCE_CLOSE(fd);
 
     VIR_FREE(xml);
 

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 168243fec94b25690151928720c8444b00399381..067890507a4ed32d4af5a0e08d9cec024d355325 100644 (file)
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -43,6 +43,7 @@
 #include "buf.h"
 #include "util.h"
 #include "memory.h"
+#include "files.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
 
@@ -1560,7 +1561,7 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
         goto cleanup;
     }
 
-    if (close(fd) < 0) {
+    if (VIR_CLOSE(fd) < 0) {
         virReportSystemError(errno,
                              _("cannot save config file %s"),
                              pool->configFile);
@@ -1570,9 +1571,7 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
     ret = 0;
 
  cleanup:
-    if (fd != -1)
-        close(fd);
-
+    VIR_FORCE_CLOSE(fd);
     VIR_FREE(xml);
 
     return ret;

diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
index 7bbdbc1871a7288a1b800df8bed40ed80b3b79b3..472a1e04b995bddd502681c58914a45f53a1a491 100644 (file)
--- a/src/conf/storage_encryption_conf.c
+++ b/src/conf/storage_encryption_conf.c
@@ -35,6 +35,7 @@
 #include "xml.h"
 #include "virterror_internal.h"
 #include "uuid.h"
+#include "files.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
 
@@ -286,12 +287,12 @@ virStorageGenerateQcowPassphrase(unsigned char *dest)
         if (r <= 0) {
             virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                                   _("Cannot read from /dev/urandom"));
-            close(fd);
+            VIR_FORCE_CLOSE(fd);
             return -1;
         }
         if (dest[i] >= 0x20 && dest[i] <= 0x7E)
             i++; /* Got an acceptable character */
     }
-    close(fd);
+    VIR_FORCE_CLOSE(fd);
     return 0;
 }

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3b127d6687daa8c73166f82be375167d2acb0792..de802f3f15a524b1d154fb4b102bcc8c14023c0e 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -769,3 +769,6 @@ virXPathLongLong;
 virXPathULongLong;
 virXPathLongHex;
 virXPathULongHex;
+
+# files.h
+virClose;

diff --git a/src/util/files.c b/src/util/files.c
new file mode 100644 (file)
index 0000000..8ccba6a
--- /dev/null
+++ b/src/util/files.c
@@ -0,0 +1,46 @@
+/*
+ * memory.c: safer file handling
+ *
+ * Copyright (C) 2010 IBM Corporation
+ * Copyright (C) 2010 Stefan Berger
+ * Copyright (C) 2010 RedHat, Inc.
+ * Copyright (C) 2010 Eric Blake
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ */
+
+#include <config.h>
+
+#include <unistd.h>
+
+#include "files.h"
+
+int virClose(int *fdptr, bool preserve_errno)
+{
+    int saved_errno;
+    int rc = 0;
+
+    if (*fdptr >= 0) {
+        if (preserve_errno)
+            saved_errno = errno;
+        rc = close(*fdptr);
+        *fdptr = -1;
+        if (preserve_errno)
+            errno = saved_errno;
+    }
+
+    return rc;
+}

diff --git a/src/util/files.h b/src/util/files.h
new file mode 100644 (file)
index 0000000..25c03e2
--- /dev/null
+++ b/src/util/files.h
@@ -0,0 +1,46 @@
+/*
+ * files.h: safer file handling
+ *
+ * Copyright (C) 2010 IBM Corporation
+ * Copyright (C) 2010 Stefan Berger
+ * Copyright (C) 2010 RedHat, Inc.
+ * Copyright (C) 2010 Eric Blake
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ */
+
+
+#ifndef __VIR_FILES_H_
+# define __VIR_FILES_H_
+
+# include <stdbool.h>
+
+# include "internal.h"
+# include "ignore-value.h"
+
+
+/* Don't call this directly - use the macros below */
+int virClose(int *fdptr, bool preserve_errno) ATTRIBUTE_RETURN_CHECK;
+
+/* For use on normal paths; caller must check return value,
+   and failure sets errno per close(). */
+# define VIR_CLOSE(FD) virClose(&(FD), false)
+
+/* For use on cleanup paths; errno is unaffected by close,
+   and no return value to worry about. */
+# define VIR_FORCE_CLOSE(FD) ignore_value(virClose(&(FD), true))
+
+#endif /* __VIR_FILES_H */