[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()



 On 10/22/2010 09:21 PM, Stefan Berger wrote:

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



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