Re: [libvirt] [PATCH v2] introduce VIR_CLOSE to be used rather than close()
libvir-list-bounces(a)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(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.
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
Attachments:
- attachment.html (text/html — 6.2 KB)