On 08/24/2017 07:23 AM, Erik Skultety wrote:
We need to perform some sanity checks on the udev monitor before every use so that we know nothing changed in the meantime. The reason for moving the code to a separate function is to be able to perform the same check from a worker thread that will replace the udevEventHandleCallback in terms of poking udev for device events.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 57 ++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 18 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f4177455c..465d272ba 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1615,47 +1615,68 @@ udevHandleOneDevice(struct udev_device *device) }
-static void -udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, - int fd, - int events ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) +/* the caller must be holding the driver lock prior to calling this function */ +static bool +udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
One line for each argument... I think in keeping with other functions - this should be named 'udevEventCheckMonitorFD'
{ - struct udev_device *device = NULL; - struct udev_monitor *udev_monitor = DRV_STATE_UDEV_MONITOR(driver); - int udev_fd = -1; + int real_fd = -1;
- udev_fd = udev_monitor_get_fd(udev_monitor); - if (fd != udev_fd) { + /* sanity check that the monitor socket hasn't changed */ + real_fd = udev_monitor_get_fd(udev_monitor); + + if (real_fd != fd) { udevPrivate *priv = driver->privateData;
virReportError(VIR_ERR_INTERNAL_ERROR, _("File descriptor returned by udev %d does not " "match node device file descriptor %d"), - fd, udev_fd); + real_fd, fd);
- /* this is a non-recoverable error, let's remove the handle, so that we - * don't get in here again because of some spurious behaviour and report - * the same error multiple times + /* this is a non-recoverable error, thus the event handle has to be + * removed, so that we don't get into the handler again because of some + * spurious behaviour */ virEventRemoveHandle(priv->watch); priv->watch = -1;
Hmmm... thinking a couple patches later - this would seem to be a good candidate for something that udevEventThreadDataFree could do (as long as it held the appropriate lock of course). It's almost too bad we couldn't somehow "pretend" to have a restart... different problem though!
- goto cleanup; + return false; }
- device = udev_monitor_receive_device(udev_monitor); - if (device == NULL) { + return true; +} + + +static void +udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, + int fd, + int events ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) +{ + struct udev_device *device = NULL; + struct udev_monitor *udev_monitor = NULL; + + nodeDeviceLock(); + udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
Technically there's a couple of things going on here - including the addition of the nodeDevice{Lock|Unlock}. Probably would have been best to make the split first, then add the Lock/Unlock afterwards (or vice versa). Still once I get to patch 3, I began wondering how the udev interaction works.
+ + if (!udevCheckMonitorFD(udev_monitor, fd)) + goto unlock;> + + if (!(device = udev_monitor_receive_device(udev_monitor))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("udev_monitor_receive_device returned NULL"));
I almost wonder if it would be better to have this be a ReportSystemError w/ errno... Not that udev docs give any guidance there, but maybe it'd help.
- goto cleanup; + goto unlock;
I know this is "existing"; however, if !device, then what's the purpose of calling udev_device_unref(NULL)? In fact there's one path in udevGetDMIData which actually checks != NULL before calling - although it has no reason to since I see no way for it to be NULL at cleanup (again a different issue). Also, wouldn't nodeDeviceLock/Unlock wrapping only be necessary within udevCheckMonitorFD? Why would the udev call need to be wrapped as well? John
}
+ nodeDeviceUnlock(); udevHandleOneDevice(device);
cleanup: udev_device_unref(device); return; + + unlock: + nodeDeviceUnlock(); + goto cleanup; }