[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] bye to close(), welcome to VIR_(FORCE_)CLOSE()




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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]