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@xxxxxxxxxx>
> ---
> 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;
> }
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
