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

15 Oct 2010

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>

---
  HACKING                            |   11 +++++++++++
  src/Makefile.am                    |    3 ++-
  src/conf/domain_conf.c             |   14 ++++++++------
  src/conf/network_conf.c            |    9 +++++----
  src/conf/nwfilter_conf.c           |   17 +++++++++--------
  src/conf/storage_conf.c            |    9 +++++----
  src/conf/storage_encryption_conf.c |    5 +++--
  src/util/files.c                   |   37 
+++++++++++++++++++++++++++++++++++++
  src/util/files.h                   |   37 
+++++++++++++++++++++++++++++++++++++
  9 files changed, 117 insertions(+), 25 deletions(-)

Index: libvirt-acl/src/util/files.c
===================================================================

--- /dev/null
+++ libvirt-acl/src/util/files.c
@@ -0,0 +1,37 @@
+/*
+ * memory.c: safer file handling
+ *
+ * Copyright (C) 2010 IBM Corporation
+ * Copyright (C) 2010 Stefan Berger
+ *
+ * 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 <unistd.h>
+
+#include "files.h"
+
+int virClose(int *fdptr)
+{
+    int rc;
+
+    if (*fdptr >= 0) {
+        rc = close(*fdptr);
+        *fdptr = -1;
+    } else
+        rc = 0;
+    return rc;
+}
Index: libvirt-acl/src/util/files.h
===================================================================
--- /dev/null
+++ libvirt-acl/src/util/files.h
@@ -0,0 +1,37 @@
+/*
+ * files.h: safer file handling
+ *
+ * Copyright (C) 2010 IBM Corporation
+ * Copyright (C) 2010 Stefan Berger
+ *
+ * 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 "internal.h"
+
+/* Don't call these directly - use the macros below */
+int virClose(int *fdptr);
+
+# define VIR_CLOSE(FD)      \
+    virClose(&(FD))
+
+
+#endif /* __VIR_FILES_H */
+
Index: libvirt-acl/src/Makefile.am
===================================================================
--- libvirt-acl.orig/src/Makefile.am
+++ libvirt-acl/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

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;
  }
@@ -7765,10 +7767,10 @@ int virDomainDiskDefForeachPath(virDomai
          }

          if (virStorageFileGetMetadataFromFD(path, fd, format, &meta) < 
0) {
-            close(fd);
+            VIR_CLOSE(fd);
              goto cleanup;
          }
-        close(fd);
+        VIR_CLOSE(fd);

          if (virHashAddEntry(paths, path, (void*)0x1) < 0) {
              virReportOOMError();
Index: libvirt-acl/src/conf/nwfilter_conf.c
===================================================================
--- libvirt-acl.orig/src/conf/nwfilter_conf.c
+++ libvirt-acl/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,19 +2194,19 @@ int virNWFilterSaveXML(const char *confi
          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;
@@ -2604,19 +2605,19 @@ virNWFilterPoolObjSaveDef(virNWFilterDri
          goto cleanup;
      }

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

      ret = 0;

   cleanup:
-    if (fd != -1)
-        close(fd);
+    VIR_CLOSE(fd);

+ cleanup_free:
      VIR_FREE(xml);

      return ret;
Index: libvirt-acl/src/conf/storage_conf.c
===================================================================
--- libvirt-acl.orig/src/conf/storage_conf.c
+++ libvirt-acl/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,19 +1561,19 @@ virStoragePoolObjSaveDef(virStorageDrive
          goto cleanup;
      }

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

      ret = 0;

   cleanup:
-    if (fd != -1)
-        close(fd);
+    VIR_CLOSE(fd);

+ cleanup_free:
      VIR_FREE(xml);

      return ret;
Index: libvirt-acl/src/conf/network_conf.c
===================================================================
--- libvirt-acl.orig/src/conf/network_conf.c
+++ libvirt-acl/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,19 +688,19 @@ int virNetworkSaveXML(const char *config
          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;
Index: libvirt-acl/src/conf/storage_encryption_conf.c
===================================================================
--- libvirt-acl.orig/src/conf/storage_encryption_conf.c
+++ libvirt-acl/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(unsigne
          if (r <= 0) {
              virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                                    _("Cannot read from /dev/urandom"));
-            close(fd);
+            VIR_CLOSE(fd);
              return -1;
          }
          if (dest[i] >= 0x20 && dest[i] <= 0x7E)
              i++; /* Got an acceptable character */
      }
-    close(fd);
+    VIR_CLOSE(fd);
      return 0;
  }
Index: libvirt-acl/HACKING
===================================================================
--- libvirt-acl.orig/HACKING
+++ libvirt-acl/HACKING
@@ -318,6 +318,17 @@ 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 filedescriptor. Instead of this API, use the macro from
+files.h
+
+   - eg close a filedescriptor
+
+       VIR_CLOSE(fd);
+

  String comparisons
  ==================

    