[libvirt] [PATCH v3 1/6] nodedev: Introduce udevCheckMonitorFD helper function

[John Ferlan]

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 at 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;
>  }
>  
>  
>