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)
 {
-    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;
 
-        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);
+
+    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"));
-        goto cleanup;
+        goto unlock;
     }
 
+    nodeDeviceUnlock();
     udevHandleOneDevice(device);
 
  cleanup:
     udev_device_unref(device);
     return;
+
+ unlock:
+    nodeDeviceUnlock();
+    goto cleanup;
 }
 
 
-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

> > +
> > +    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.

Not really, that would imply that we can rely on libudev setting the errno, but
the call can fail in multiple ways the most of which are unrelated to any
system call and given the poor interface libudev has :/ we don't have much of a
choice (but yeah, the error is rather uninformative, but since you have no
control over it, it's the best we have IMO).

>
> > -        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?

So, IMHO, again, I'm still not convinced about the whole file descriptor
changing under our hands idea (unrelated - was it actually the correct
wording?). But since the general consensus was to keep the sanity checks, I
moved the @fd extraction within the locks. Now, if we presume such thing can
happen, you don't want anyone touching the driver structure in the meantime,
otherwise you're left with a dangling pointer @udev_monitor and bad things
would happen. This way, you don't have to worry about the driver's structure
getting changed, possibly invalidating the file descriptor (not that we were,
but if we really would/should...).

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list