[libvirt] [PATCH v2] introduce VIR_CLOSE to be used rather than close()
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@us.ibm.com> --- HACKING | 17 +++++++++++++ docs/hacking.html.in | 22 +++++++++++++++++ src/Makefile.am | 3 +- src/conf/domain_conf.c | 15 +++++++---- src/conf/network_conf.c | 7 +++-- src/conf/nwfilter_conf.c | 13 ++++------ src/conf/storage_conf.c | 8 +++--- src/conf/storage_encryption_conf.c | 6 +++- src/util/files.c | 47 +++++++++++++++++++++++++++++++++++++ src/util/files.h | 45 +++++++++++++++++++++++++++++++++++ 10 files changed, 161 insertions(+), 22 deletions(-) Index: libvirt-acl/src/util/files.c =================================================================== --- /dev/null +++ libvirt-acl/src/util/files.c @@ -0,0 +1,47 @@ +/* + * 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; + + if (*fdptr >= 0) { + if (preserve_errno) + saved_errno = errno; + rc = close(*fdptr); + *fdptr = -1; + if (preserve_errno) + errno = saved_errno; + } else + rc = 0; + + return rc; +} Index: libvirt-acl/src/util/files.h =================================================================== --- /dev/null +++ libvirt-acl/src/util/files.h @@ -0,0 +1,45 @@ +/* + * 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" + + +/* 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 @@ -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 *configD 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 *configD ret = 0; cleanup: - if (fd != -1) - close(fd); + VIR_FORCE_CLOSE(fd); + VIR_FREE(configFile); return ret; } @@ -7765,10 +7766,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,8 @@ #include "nwfilter_conf.h" #include "domain_conf.h" #include "c-ctype.h" +#include "files.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -2193,7 +2195,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 +2205,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 +2604,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 +2614,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,8 @@ #include "buf.h" #include "util.h" #include "memory.h" +#include "files.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1560,7 +1562,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 +1572,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,8 @@ #include "util.h" #include "buf.h" #include "c-ctype.h" +#include "files.h" +#include "ignore-value.h" #define MAX_BRIDGE_ID 256 #define VIR_FROM_THIS VIR_FROM_NETWORK @@ -687,7 +689,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 +699,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,8 @@ #include "xml.h" #include "virterror_internal.h" #include "uuid.h" +#include "files.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -286,12 +288,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 filedescriptor. 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; this function + preserves the error code + + 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,29 @@ </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 filedescriptor. 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; this function preserves the error code</p> + +<pre> + VIR_FORCE_CLOSE(fd); +</pre></li> + </ul> <h2><a name="string_comparision">String comparisons</a></h2>
On 10/15/2010 01:10 PM, Stefan Berger wrote:
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@us.ibm.com>
--- HACKING | 17 +++++++++++++ docs/hacking.html.in | 22 +++++++++++++++++ src/Makefile.am | 3 +- src/conf/domain_conf.c | 15 +++++++---- src/conf/network_conf.c | 7 +++-- src/conf/nwfilter_conf.c | 13 ++++------ src/conf/storage_conf.c | 8 +++--- src/conf/storage_encryption_conf.c | 6 +++- src/util/files.c | 47 +++++++++++++++++++++++++++++++++++++ src/util/files.h | 45 +++++++++++++++++++++++++++++++++++ 10 files changed, 161 insertions(+), 22 deletions(-)
Make sure you include an edit to libvirt_private.syms before pushing, but that should be simple enough that I don't need to see a v3 for just that issue.
+int virClose(int *fdptr, bool preserve_errno) +{ + int saved_errno; + int rc; + + if (*fdptr >= 0) { + if (preserve_errno) + saved_errno = errno; + rc = close(*fdptr); + *fdptr = -1; + if (preserve_errno) + errno = saved_errno; + } else + rc = 0;
Style nit: HACKING discourages the style: if () { ... } else one-line; and recommends either: if () { ... } else { one-line; } if (!) one-line; else { ... } Or better yet, avoiding the else in the first place, by initializing rc to begin with: int rc = 0; if () { ... rc = close() ... } return rc;
+# include <stdbool.h> + +# include "internal.h"
You need #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))
for this to work as a self-contained header. Style nits: ignore_value(virClose(&(FD), true)) ^ ^ space after comma no space on function call
--- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -45,6 +45,8 @@ #include "nwfilter_conf.h" #include "domain_conf.h" #include "c-ctype.h" +#include "files.h" +#include "ignore-value.h"
Clients shouldn't be including the "ignore-value.h" header unless they are directly using it (hmm, wondering if this is another 'make syntax-check' rule we should be enforcing).
+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
s/filedescriptor/file descriptor/
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; this function + preserves the error code
That reads a bit ambiguously for me (preserves which error code? the one from close(), or from before VIR_CLOSE?). How about: close a file descriptor in an error path, without losing the previous errno value
+ <p> + Use of the close() API is deprecated in libvirt code base to help + avoiding double-closing of a filedescriptor. Instead of this API,
s/filedescriptor/file descriptor/ ACK with the nits addressed; I think you are close enough without needing a v3. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
libvir-list-bounces@redhat.com wrote on 10/15/2010 05:35:33 PM:
On 10/15/2010 01:10 PM, Stefan Berger wrote:
V2: - following Eric's suggestions and picking up his code suggestions
Since bugs due to double-closed file descriptors are difficult to
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
track base
(HACKING).
Signed-off-by: Stefan Berger <stefanb@us.ibm.com>
--- HACKING | 17 +++++++++++++ docs/hacking.html.in | 22 +++++++++++++++++ src/Makefile.am | 3 +- src/conf/domain_conf.c | 15 +++++++---- src/conf/network_conf.c | 7 +++-- src/conf/nwfilter_conf.c | 13 ++++------ src/conf/storage_conf.c | 8 +++--- src/conf/storage_encryption_conf.c | 6 +++- src/util/files.c | 47 +++++++++++++++++++++++++++++++++++++ src/util/files.h | 45 +++++++++++++++++++++++++++++++++++ 10 files changed, 161 insertions(+), 22 deletions(-)
Make sure you include an edit to libvirt_private.syms before pushing, but that should be simple enough that I don't need to see a v3 for just that issue.
Ok.
}
Or better yet, avoiding the else in the first place, by initializing rc to begin with:
int rc = 0;
if () { ... rc = close() ... } return rc;
Ok, will change it to this.
+# include <stdbool.h> + +# include "internal.h"
You need #include "ignore-value.h"...
The problem with this include file is that it doesn't protect itself from multiple inclusion with a #ifndef, #define sequence, so I ended up getting re-definitions of ignore_value. So I pushed the #include into the .c files. Well, let me know whether you agree and I'll push with the nits addressed.
+ + +/* 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))
for this to work as a self-contained header. Style nits:
ignore_value(virClose(&(FD), true)) ^ ^ space after comma no space on function call
Ok.
--- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -45,6 +45,8 @@ #include "nwfilter_conf.h" #include "domain_conf.h" #include "c-ctype.h" +#include "files.h" +#include "ignore-value.h"
Clients shouldn't be including the "ignore-value.h" header unless they are directly using it (hmm, wondering if this is another 'make syntax-check' rule we should be enforcing).
... so goes the theory, 'yes'. We could define ignore_value in the same way as gnulib does and the problem with that include file would be gone.
+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
s/filedescriptor/file descriptor/
Ok.
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; this function + preserves the error code
That reads a bit ambiguously for me (preserves which error code? the one
from close(), or from before VIR_CLOSE?). How about:
close a file descriptor in an error path, without losing the previous errno value
+ <p> + Use of the close() API is deprecated in libvirt code base to help + avoiding double-closing of a filedescriptor. Instead of this API,
s/filedescriptor/file descriptor/
Ok.
ACK with the nits addressed; I think you are close enough without needing a v3.
Many changes .. I'll post a V3. Stefan
[adding bug-gnulib] On 10/15/2010 03:58 PM, Stefan Berger wrote:
+# include<stdbool.h> + +# include "internal.h"
You need #include "ignore-value.h"...
The problem with this include file is that it doesn't protect itself from multiple inclusion with a #ifndef, #define sequence, so I ended up getting re-definitions of ignore_value. So I pushed the #include into the .c files.
Oh, I missed that. It's always easier, maintenance wise, for every header to be idempotent, so let's fix this in gnulib.
Well, let me know whether you agree and I'll push with the nits addressed.
Hmm; maybe it's easier to wait for the gnulib update to go in first.
Many changes .. I'll post a V3.
Good idea. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* .gnulib: Update to latest. --- Let's push this first, although that means you may want a v4 to move the include "ignore-value.h" line around. * .gnulib b6d1430...cac3889 (8):
ignore-value: make header idempotent GNUmakefile: handle "stable" target, not "major" isnan: Add support for TinyCC vasnprintf: Don't set errno to 0. socketlib: Fix. test-select-stdin.c: avoid warn_unused_result warnings git-version-gen: do require git-VC'd files in cwd argv-iter: omit nonconforming declaration
.gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/.gnulib b/.gnulib index b6d1430..cac3889 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit b6d1430494cdd252cd52eca6abf88b1a00f6c983 +Subproject commit cac3889c1830d38e5b55ae69fc3d458498a0b33e -- 1.7.2.3
On 10/15/2010 06:14 PM, Eric Blake wrote:
* .gnulib: Update to latest. ---
Let's push this first, although that means you may want a v4 to move the include "ignore-value.h" line around.
* .gnulib b6d1430...cac3889 (8):
ignore-value: make header idempotent GNUmakefile: handle "stable" target, not "major" isnan: Add support for TinyCC vasnprintf: Don't set errno to 0. socketlib: Fix. test-select-stdin.c: avoid warn_unused_result warnings git-version-gen: do require git-VC'd files in cwd argv-iter: omit nonconforming declaration
.gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/.gnulib b/.gnulib index b6d1430..cac3889 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit b6d1430494cdd252cd52eca6abf88b1a00f6c983 +Subproject commit cac3889c1830d38e5b55ae69fc3d458498a0b33e ACK Stefan
On 10/15/2010 05:07 PM, Stefan Berger wrote:
Let's push this first, although that means you may want a v4 to move the include "ignore-value.h" line around.
-Subproject commit b6d1430494cdd252cd52eca6abf88b1a00f6c983 +Subproject commit cac3889c1830d38e5b55ae69fc3d458498a0b33e ACK
Now pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Stefan Berger -
Stefan Berger