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

Re: [libvirt] [PATCH] nodedev: udev: Drop udev monitor fd check in udevEventHandleCallback



On Mon, Jun 26, 2017 at 01:12:03PM +0100, Daniel P. Berrange wrote:
> On Mon, Jun 26, 2017 at 01:47:04PM +0200, Erik Skultety wrote:
> > We do perform that magical check every time an event is received, but
> > unless we explicitly unref'd or disconnected the monitor, we should not
> > get a spurious event from a different source.
> >
> > Signed-off-by: Erik Skultety <eskultet redhat com>
> > ---
> > This is just a poke patch, since I also checked libudev's source and still fail
> > to understand why we need that check. Unless entered by *_monitor_unref with
> > the last reference or with *monitor_disconnect, libudev doesn't change the fd
> > arbitrarily. So the only reason for us to check for that would be if we
> > disconnected from our side, but didn't unregister the callback in which case
> > it was our responsibility to clean up properly and we shouldn't get to the
> > callback in the first place. Thanks for comments in advance.
>
> This check is simply a sanity check that we've not got a bug elsewhere
> in the code. If we're bug free, this condition should never trigger.
>
> This is probably better thought of as being an assert(), except we don't
> use assert() hence reporting an error.
>
> We have certainly had bugs in the past where we didn't correctly cleanup
> or unregister things, so I think the check is potentially useful.

OK, I take that, but if we actually ever hit that check, we should make sure we
don't get there again, e.g. by getting the same fd from open() and friends, so
we should unregister the cb on error.

>
> >  src/node_device/node_device_udev.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index 174124a1b..7820c49c4 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1602,24 +1602,15 @@ nodeStateCleanup(void)
> >
> >  static void
> >  udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> > -                        int fd,
> > +                        int fd ATTRIBUTE_UNUSED,
> >                          int events ATTRIBUTE_UNUSED,
> >                          void *data ATTRIBUTE_UNUSED)
> >  {
> >      struct udev_device *device = NULL;
> >      struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> >      const char *action = NULL;
> > -    int udev_fd = -1;
> >
> >      nodeDeviceLock();
> > -    udev_fd = udev_monitor_get_fd(udev_monitor);
> > -    if (fd != udev_fd) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("File descriptor returned by udev %d does not "
> > -                         "match node device file descriptor %d"),
> > -                       fd, udev_fd);
> > -        goto cleanup;
> > -    }
> >
> >      device = udev_monitor_receive_device(udev_monitor);
> >      if (device == NULL) {
> > --
> > 2.13.2
> >
> > --
> > libvir-list mailing list
> > libvir-list redhat com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


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