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&gt; --- 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) &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> 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;