V4:
- pushing #include "ignore-value.h" into files.h
V3:
- many small nits addressed
V2:
- following Eric's suggestions and picking up his code suggestions
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).
Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>
---
HACKING | 17 +++++++++++++
docs/hacking.html.in | 23 ++++++++++++++++++
src/Makefile.am | 3 +-
src/conf/domain_conf.c | 16 ++++++++----
src/conf/network_conf.c | 6 ++--
src/conf/nwfilter_conf.c | 12 ++++-----
src/conf/storage_conf.c | 7 ++---
src/conf/storage_encryption_conf.c | 5 ++--
src/libvirt_private.syms | 3 ++
src/util/files.c | 46
+++++++++++++++++++++++++++++++++++++
src/util/files.h | 46
+++++++++++++++++++++++++++++++++++++
11 files changed, 161 insertions(+), 23 deletions(-)
Index: libvirt-acl/src/util/files.c
===================================================================
--- /dev/null
+++ libvirt-acl/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;
+}
Index: libvirt-acl/src/util/files.h
===================================================================
--- /dev/null
+++ libvirt-acl/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 */
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
@@ -44,8 +44,8 @@
#include "network.h"
#include "macvtap.h"
#include "nwfilter_conf.h"
-#include "ignore-value.h"
#include "storage_file.h"
+#include "files.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -6798,7 +6798,7 @@ int virDomainSaveXML(const char *configD
goto cleanup;
}
- if (close(fd) < 0) {
+ if (VIR_CLOSE(fd) < 0) {
virReportSystemError(errno,
_("cannot save config file '%s'"),
configFile);
@@ -6807,8 +6807,8 @@ int virDomainSaveXML(const char *configD
ret = 0;
cleanup:
- if (fd != -1)
- close(fd);
+ VIR_FORCE_CLOSE(fd);
+
VIR_FREE(configFile);
return ret;
}
@@ -7765,10 +7765,14 @@ int virDomainDiskDefForeachPath(virDomai
}
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();
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,7 +2194,7 @@ int virNWFilterSaveXML(const char *confi
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 *confi
ret = 0;
cleanup:
- if (fd != -1)
- close(fd);
-
+ VIR_FORCE_CLOSE(fd);
VIR_FREE(configFile);
return ret;
@@ -2604,7 +2603,7 @@ virNWFilterPoolObjSaveDef(virNWFilterDri
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(virNWFilterDri
ret = 0;
cleanup:
- if (fd != -1)
- close(fd);
+ VIR_FORCE_CLOSE(fd);
VIR_FREE(xml);
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,7 +1561,7 @@ virStoragePoolObjSaveDef(virStorageDrive
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(virStorageDrive
ret = 0;
cleanup:
- if (fd != -1)
- close(fd);
-
+ VIR_FORCE_CLOSE(fd);
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,7 +688,7 @@ int virNetworkSaveXML(const char *config
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 *config
ret = 0;
cleanup:
- if (fd != -1)
- close(fd);
+ VIR_FORCE_CLOSE(fd);
VIR_FREE(configFile);
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_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;
}
Index: libvirt-acl/HACKING
===================================================================
--- libvirt-acl.orig/HACKING
+++ libvirt-acl/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
==================
Index: libvirt-acl/docs/hacking.html.in
===================================================================
--- libvirt-acl.orig/docs/hacking.html.in
+++ libvirt-acl/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) < 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>
Index: libvirt-acl/src/libvirt_private.syms
===================================================================
--- libvirt-acl.orig/src/libvirt_private.syms
+++ libvirt-acl/src/libvirt_private.syms
@@ -769,3 +769,6 @@ virXPathLongLong;
virXPathULongLong;
virXPathLongHex;
virXPathULongHex;
+
+# files.h
+virClose;
