Re: [libvirt] [PATCH v2] introduce VIR_CLOSE to be used rather than close()
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org