9:30 p.m.
On 10/22/2010 09:21 PM, Stefan Berger wrote:
libvir-list-bounces(a)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).