libvir-list
On 10/22/2010 05:19 AM, Stefan Berger wrote:
Using automated replacement with sed and editing I have now replaced all occurrences of close() with VIR_(FORCE_)CLOSE() except for one, of course. Some replacements were straight forward, others I needed to
libvir-list-bounces@redhat.com wrote on 10/22/2010 06:27:30 PM: pay
attention. I hope I payed attention in all the right places... Please have a look. This should have at least solved one more double-close error.
Can you isolate any of those double-close errors into separate patches which we can apply now, rather than drowning them in the giant patch?
Signed-off-by: Stefan Berger<stefanb@us.ibm.com>
src/phyp/phyp_driver.c | 13 ++--
Resuming...
[...]
Index: libvirt-acl/src/qemu/qemu_monitor.c
@@ -694,8 +695,7 @@ void qemuMonitorClose(qemuMonitorPtr mon if (!mon->closed) { if (mon->watch) virEventRemoveHandle(mon->watch); - if (mon->fd != -1) - close(mon->fd); + VIR_FORCE_CLOSE(mon->fd); /* NB: ordinarily one might immediately set mon->watch to -1 * and mon->fd to -1, but there may be a callback active * that is still relying on these fields being valid. So
Ouch - given that comment, could we be frying a callback by setting mon->fd to -1 in VIR_FORCE_CLOSE? We need to double check this, and possibly use a temporary variable if the callback indeed needs a non-negative mon->fd for a bit longer, or tighten the specification and all existing callbacks to tolerate mon->fd changing to -1.
Probably worth splitting this particular hunk into its own commit, rather than part of the giant patch.
Yes, good idea. Obviously I did not read the comment.
Index: libvirt-acl/src/remote/remote_driver.c =================================================================== --- libvirt-acl.orig/src/remote/remote_driver.c +++ libvirt-acl/src/remote/remote_driver.c @@ -82,6 +82,7 @@ #include "util.h" #include "event.h" #include "ignore-value.h" +#include "files.h"
#define VIR_FROM_THIS VIR_FROM_REMOTE
@@ -711,7 +712,7 @@ doRemoteOpen (virConnectPtr conn, if (errno == ECONNREFUSED&& flags& VIR_DRV_OPEN_REMOTE_AUTOSTART&& trials< 20) { - close(priv->sock); + VIR_FORCE_CLOSE(priv->sock); priv->sock = -1;
This line is now redundant.
Done.
Index: libvirt-acl/src/uml/uml_driver.c @@ -811,7 +811,7 @@ static int umlStartVMDaemon(virConnectPt virDomainObjPtr vm) { const char **argv = NULL, **tmp; const char **progenv = NULL; - int i, ret; + int i, ret, tmpfd; pid_t pid; char *logfile; int logfd = -1;
for (i = 0; i< FD_SETSIZE; i++) - if (FD_ISSET(i,&keepfd)) - close(i); + if (FD_ISSET(i,&keepfd)) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd);
Another awfully large scope for a needed temporary.
Ok.
Index: libvirt-acl/src/util/bridge.c @@ -107,7 +108,7 @@ brShutdown(brControl *ctl) if (!ctl) return;
- close(ctl->fd); + VIR_FORCE_CLOSE(ctl->fd); ctl->fd = 0;
Huh - is this an existing logic bug? Can we end up accidentally double-closing stdin?
I'ld leave the patch for now as it is, i.e., do the VIR_FORCE_CLOSE and remember to investigate. So far, the changed does not have any further negative impact that already isn't there - but it also doesn't solve a potential problem.
Index: libvirt-acl/src/util/logging.c =================================================================== --- libvirt-acl.orig/src/util/logging.c +++ libvirt-acl/src/util/logging.c @@ -40,6 +40,7 @@ #include "util.h" #include "buf.h" #include "threads.h" +#include "files.h"
/* * Macro used to format the message as a string in virLogMessage @@ -603,8 +604,7 @@ static int virLogOutputToFd(const char * static void virLogCloseFd(void *data) { int fd = (long) data;
- if (fd>= 0) - close(fd); + VIR_FORCE_CLOSE(fd);
Should we fix this function to return an int value, and return VIR_CLOSE(fd) so that callers can choose to detect log close failures?
Also that I would delay until further 'cause this may have consequences for those calling the function.
Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -52,6 +52,7 @@ # include "conf/domain_conf.h" # include "virterror_internal.h" # include "uuid.h" +# include "files.h"
# define VIR_FROM_THIS VIR_FROM_NET
@@ -94,7 +95,7 @@ static int nlOpen(void)
static void nlClose(int fd) { - close(fd); + VIR_FORCE_CLOSE(fd);
Likewise?
This here is a close of a netlink socket, which was used to communicate with the kernel. I would just close it and discard the returned value.
Index: libvirt-acl/src/util/util.c =================================================================== --- libvirt-acl.orig/src/util/util.c +++ libvirt-acl/src/util/util.c @@ -71,6 +71,7 @@ #include "memory.h" #include "threads.h" #include "verify.h" +#include "files.h"
#ifndef NSIG # define NSIG 32 @@ -461,6 +462,7 @@ __virExec(const char *const*argv, int pipeerr[2] = {-1,-1}; int childout = -1; int childerr = -1; + int tmpfd;
@@ -568,8 +570,10 @@ __virExec(const char *const*argv, i != childout&& i != childerr&& (!keepfd || - !FD_ISSET(i, keepfd))) - close(i); + !FD_ISSET(i, keepfd))) { + tmpfd = i; + VIR_FORCE_CLOSE(tmpfd);
I thought this was another large scope for a temporary....
+ }
if (dup2(infd>= 0 ? infd : null, STDIN_FILENO)< 0) { virReportSystemError(errno, @@ -589,14 +593,15 @@ __virExec(const char *const*argv, goto fork_error; }
- if (infd> 0) - close(infd); - close(null); - if (childout> 0) - close(childout); + VIR_FORCE_CLOSE(infd); + VIR_FORCE_CLOSE(null); + tmpfd = childout; /* preserve childout value */ + VIR_FORCE_CLOSE(tmpfd);
...but you needed it here too.
Yes, here I need it twice. So not changing it.
if (childerr> 0&& - childerr != childout) - close(childerr); + childerr != childout) { + VIR_FORCE_CLOSE(childerr); + childout = -1; + }
Looks okay after all - certainly not one of the trivial conversions though :)
Index: libvirt-acl/src/util/virtaudit.c =================================================================== --- libvirt-acl.orig/src/util/virtaudit.c +++ libvirt-acl/src/util/virtaudit.c @@ -30,6 +30,7 @@ #include "virterror_internal.h" #include "logging.h" #include "virtaudit.h" +#include "files.h"
/* Provide the macros in case the header file is old. FIXME: should be removed. */ @@ -133,6 +134,6 @@ void virAuditSend(const char *file ATTRI void virAuditClose(void) { #if HAVE_AUDIT - close(auditfd); + VIR_CLOSE(auditfd);
Must be VIR_FORCE_CLOSE; I'm guessing you didn't have audit turned on in
your compile.
Right...
Index: libvirt-acl/src/xen/proxy_internal.c @@ -236,12 +237,11 @@ virProxyCloseSocket(xenUnifiedPrivatePtr if (priv->proxy< 0) return(-1);
- ret = close(priv->proxy); + ret = VIR_CLOSE(priv->proxy); if (ret != 0) VIR_WARN("Failed to close socket %d", priv->proxy); else VIR_DEBUG("Closed socket %d", priv->proxy);
Subtle unintended semantic change; you're now printing -1 instead of the
fd you just closed; you'll need a temporary.
There was another one like this that I got right :)
Index: libvirt-acl/src/xen/xend_internal.c @@ -137,9 +137,7 @@ do_connect(virConnectPtr xend)
if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) ==
-1) {
- serrno = errno; - close(s); - errno = serrno; + VIR_FORCE_CLOSE(s); s = -1;
Redundant line. Overall, I'm impressed with how many lines this is shaving off the code base! I think we settled on a pretty good calling convention.
Fixed. Will post v2 and if useful a diff(v1,v2). Stefan
And with that, I've completed my review of v1.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list