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

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(a)redhat.com&gt; > --- > 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.

> 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(a)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 :|