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