<tt>libvir-list-bounces@redhat.com wrote on 10/22/2010 06:27:30 PM: > libvir-list</tt> <tt>> > 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 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... > </tt> <tt>[...] > > > 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.</tt> <tt>Yes, good idea. Obviously I did not read the comment.</tt> <tt> > > > 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.</tt> <tt>Done.</tt> <tt> > > > 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.</tt> <tt>Ok.</tt> <tt> > > > 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?</tt> <tt>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.</tt> <tt> > > > 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?</tt> <tt>Also that I would delay until further 'cause this may have consequences for those calling the function.</tt> <tt> > > > 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?</tt> <tt>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.</tt> <tt> > > > 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.</tt> <tt>Yes, here I need it twice. So not changing it.</tt> <tt> > > > 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.</tt> <tt>Right...</tt> <tt> > > > 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.</tt> <tt>There was another one like this that I got right :) </tt> <tt>> > > 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.</tt> <tt>Fixed. Will post v2 and if useful a diff(v1,v2).</tt> <tt> Stefan</tt> <tt> > > And with that, I've completed my review of v1. > > -- > Eric Blake eblake@redhat.com +1-801-349-2682 > Libvirt virtualization library </tt><a href=http://libvirt.org/><tt>http://libvirt.org</tt></a><tt> > > -- > libvir-list mailing list > libvir-list@redhat.com > </tt><a href="https://www.redhat.com/mailman/listinfo/libvir-list"><tt>https://www.redhat.com/mailman/listinfo/libvir-list</tt></a><tt> </tt>