1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v4 0/7] Work around the kernel mdev uevent race in nodedev
  4. View patch

[libvirt] [PATCH v4 1/7] nodedev: Introduce udevEventCheckMonitorFD helper function

Erik Skultety posted 7 patches 8 years, 2 months ago
There is a newer version of this series
[libvirt] [PATCH v4 1/7] nodedev: Introduce udevEventCheckMonitorFD helper function
Posted by Erik Skultety 8 years, 2 months ago 
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 | 40 +++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index f4177455c..fe21ad7df 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1615,14 +1615,10 @@ udevHandleOneDevice(struct udev_device *device)
 }
 
 
-static void
-udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
-                        int fd,
-                        int events ATTRIBUTE_UNUSED,
-                        void *data ATTRIBUTE_UNUSED)
+static bool
+udevEventCheckMonitorFD(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;
 
     udev_fd = udev_monitor_get_fd(udev_monitor);
@@ -1641,21 +1637,39 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
         virEventRemoveHandle(priv->watch);
         priv->watch = -1;
 
-        goto cleanup;
+        return false;
+    }
+
+    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;
+
+    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
+
+    if (!udevEventCheckMonitorFD(udev_monitor, fd)) {
+        nodeDeviceUnlock();
+        return;
     }
 
     device = udev_monitor_receive_device(udev_monitor);
-    if (device == NULL) {
+
+    if (!device) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("udev_monitor_receive_device returned NULL"));
-        goto cleanup;
+        return;
     }
 
     udevHandleOneDevice(device);
-
- cleanup:
     udev_device_unref(device);
-    return;
 }
 
 
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/7] nodedev: Introduce udevEventCheckMonitorFD helper function
Posted by John Ferlan 8 years, 2 months ago 

On 09/18/2017 12:34 PM, 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 | 40 +++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index f4177455c..fe21ad7df 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1615,14 +1615,10 @@ udevHandleOneDevice(struct udev_device *device)
>  }
>  
>  
> -static void
> -udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> -                        int fd,
> -                        int events ATTRIBUTE_UNUSED,
> -                        void *data ATTRIBUTE_UNUSED)
> +static bool
> +udevEventCheckMonitorFD(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;
>  
>      udev_fd = udev_monitor_get_fd(udev_monitor);
> @@ -1641,21 +1637,39 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>          virEventRemoveHandle(priv->watch);
>          priv->watch = -1;
>  
> -        goto cleanup;
> +        return false;
> +    }
> +
> +    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;
> +
> +    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> +
> +    if (!udevEventCheckMonitorFD(udev_monitor, fd)) {
> +        nodeDeviceUnlock();

Me thinks this particular line belongs in the next patch... of course
that means the { } won't be necessary here.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> +        return;
>      }
>  
>      device = udev_monitor_receive_device(udev_monitor);
> -    if (device == NULL) {
> +
> +    if (!device) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("udev_monitor_receive_device returned NULL"));
> -        goto cleanup;
> +        return;
>      }
>  
>      udevHandleOneDevice(device);
> -
> - cleanup:
>      udev_device_unref(device);
> -    return;
>  }
>  
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/7] nodedev: Introduce udevEventCheckMonitorFD helper function
Posted by Erik Skultety 8 years, 2 months ago 
On Wed, Sep 20, 2017 at 08:52:26AM -0400, John Ferlan wrote:
>
>
> On 09/18/2017 12:34 PM, 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 | 40 +++++++++++++++++++++++++-------------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index f4177455c..fe21ad7df 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1615,14 +1615,10 @@ udevHandleOneDevice(struct udev_device *device)
> >  }
> >
> >
> > -static void
> > -udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> > -                        int fd,
> > -                        int events ATTRIBUTE_UNUSED,
> > -                        void *data ATTRIBUTE_UNUSED)
> > +static bool
> > +udevEventCheckMonitorFD(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;
> >
> >      udev_fd = udev_monitor_get_fd(udev_monitor);
> > @@ -1641,21 +1637,39 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> >          virEventRemoveHandle(priv->watch);
> >          priv->watch = -1;
> >
> > -        goto cleanup;
> > +        return false;
> > +    }
> > +
> > +    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;
> > +
> > +    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> > +
> > +    if (!udevEventCheckMonitorFD(udev_monitor, fd)) {
> > +        nodeDeviceUnlock();
>
> Me thinks this particular line belongs in the next patch... of course
> that means the { } won't be necessary here.

Do-oh! Right, I'll move it - that's just a result of 4+ rebases since I
couldn't make up my mind on how to split it in the least disruptive way so that
I'd make the reviewer's job much easier.

Thanks,
Erik

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.