[libvirt] [PATCH v2] introduce VIR_CLOSE to be used rather than close()

[Eric Blake]

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 at 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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org