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

[Laine Stump]

  On 10/22/2010 09:21 PM, Stefan Berger wrote:
>
> libvir-list-bounces at redhat.com (Eric Blake) wrote on 10/22/2010 
> 06:27:30 PM:
>
>
> [...]
> >
> > > 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.


The only possible thing I could think a callback might legitimately use 
a non-negative fd for would be to answer the question "was this file 
previously successfully opened?" I would say that if it's being used for 
that, it would be cleaner to have a separate flag set in the qemuMonitor 
object that was set when the fd was opened, and never reset.

Any other use of the value of fd after it's closed that I can think of 
is a bug, and changing fd to -1 here would hopefully reveal that bug so 
it could be fixed.

> >
> >
> > > 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?


Am I missing something? Sure, ctl->fd is set to 0, but then the memory 
at ctl is immediately freed (in the next line, which doesn't show up in 
the patch diff), and never referenced again. I'm not even sure why they 
bothered to set ctl->fd to 0, since it's guaranteed to never be used again.


> I'ld leave the patch for now as it is, i.e., do the VIR_FORCE_CLOSE 
> and remember to investigate.


I think we can/should remove both the close() and the ctl->fd = 0, and 
replace them with VIR_FORCE_CLOSE().


> 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.


Agreed. That's a good idea, but should be a separate patch.


>
> >
> > > 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.


I kind of agree with that, but since a close failure should never 
happen, when it does happen it may very well indicate something "Very 
Bad" (eg, corrupted memory, leading to eventual exhaustion of nl sockets 
(which turns out doesn't take very long)), so we might want to log an 
error just so they'll know. However, the error could just as well be 
logged right here, as to return the status to the caller. (And again, I 
think it can be a separate patch).