[libvirt] [PATCH] bye to close(), welcome to VIR_(FORCE_)CLOSE()
Stefan Berger
stefanb at us.ibm.com
Sat Oct 23 01:21:31 UTC 2010
libvir-list-bounces at redhat.com wrote on 10/22/2010 06:27:30 PM:
> 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
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 at 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 at redhat.com +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101022/6ac58d35/attachment-0001.htm>
More information about the libvir-list
mailing list