[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]