libvir-list-bounces@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@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