From f04de501bca81d894441905fc53b5efbb3485134 Mon Sep 17 00:00:00 2001 From: Stefan Berger Date: Tue, 19 Oct 2010 10:23:51 -0400 Subject: [PATCH] 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). --- HACKING | 17 +++++++++++ docs/hacking.html.in | 23 +++++++++++++++ src/Makefile.am | 3 +- src/conf/domain_conf.c | 15 ++++++---- 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(+), 22 deletions(-) create mode 100644 src/util/files.c create mode 100644 src/util/files.h diff --git a/HACKING b/HACKING index e22d73c71e..a9a9b49924 100644 --- 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 deab530abc..bd8b443a94 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -389,7 +389,30 @@ +

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 +

+ +

String comparisons

diff --git a/src/Makefile.am b/src/Makefile.am index 9bc42872c3..e386bfafaa 100644 --- 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 ad8085f79f..78d7a6ad5b 100644 --- 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 4c0248c5df..ddef7909d6 100644 --- 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 a553b51648..40fbf5e2f4 100644 --- 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 168243fec9..067890507a 100644 --- 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 7bbdbc1871..472a1e04b9 100644 --- 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 3b127d6687..de802f3f15 100644 --- 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 index 0000000000..8ccba6ad7d --- /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 + +#include + +#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 index 0000000000..25c03e245d --- /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 + +# 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 */