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

[Stefan Berger]

libvir-list-bounces at 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 
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.
> 

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101015/28cc87cf/attachment-0001.htm>