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

[libvirt] [PATCH v3 2/6] udev: Split udevEventHandleCallback in two functions

Erik Skultety posted 6 patches 8 years, 2 months ago
There is a newer version of this series
[libvirt] [PATCH v3 2/6] udev: Split udevEventHandleCallback in two functions
Posted by Erik Skultety 8 years, 2 months ago 
This patch splits udevEventHandleCallback in two (introduces
udevEventHandleThread) in order to be later able to refactor the latter
to actually become a detached thread which will wait some time for the
kernel to create the whole sysfs tree for a device as we cannot do that
in the event loop directly.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/node_device/node_device_udev.c | 54 ++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 465d272ba..fe943c78b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1647,36 +1647,56 @@ udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
 
 
 static void
+udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
+{
+    struct udev_device *device = NULL;
+    struct udev_monitor *udev_monitor = NULL;
+    int fd = (intptr_t) opaque;
+
+    nodeDeviceLock();
+    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
+
+    if (!udevCheckMonitorFD(udev_monitor, fd))
+        goto unlock;
+
+    device = udev_monitor_receive_device(udev_monitor);
+    if (device == NULL) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("udev_monitor_receive_device returned NULL"));
+        goto unlock;
+    }
+
+    nodeDeviceUnlock();
+    udevHandleOneDevice(device);
+
+ cleanup:
+    udev_device_unref(device);
+    return;
+
+ unlock:
+    nodeDeviceUnlock();
+    goto cleanup;
+}
+
+
+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 unlock;
+    if (!udevCheckMonitorFD(udev_monitor, fd)) {
+        nodeDeviceUnlock();
+        return;
     }
-
     nodeDeviceUnlock();
-    udevHandleOneDevice(device);
-
- cleanup:
-    udev_device_unref(device);
-    return;
 
- unlock:
-    nodeDeviceUnlock();
-    goto cleanup;
+    udevEventHandleThread((void *)(intptr_t) fd);
 }
 
 
-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/6] udev: Split udevEventHandleCallback in two functions
Posted by John Ferlan 8 years, 2 months ago 

On 08/24/2017 07:23 AM, Erik Skultety wrote:
> This patch splits udevEventHandleCallback in two (introduces
> udevEventHandleThread) in order to be later able to refactor the latter
> to actually become a detached thread which will wait some time for the
> kernel to create the whole sysfs tree for a device as we cannot do that
> in the event loop directly.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 54 ++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 465d272ba..fe943c78b 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1647,36 +1647,56 @@ udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
>  
>  
>  static void
> +udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)

It's not ATTRIBUTE_UNUSED

> +{
> +    struct udev_device *device = NULL;
> +    struct udev_monitor *udev_monitor = NULL;
> +    int fd = (intptr_t) opaque;

^^^  opaque!

> +
> +    nodeDeviceLock();
> +    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> +
> +    if (!udevCheckMonitorFD(udev_monitor, fd))
> +        goto unlock;
> +
> +    device = udev_monitor_receive_device(udev_monitor);
> +    if (device == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("udev_monitor_receive_device returned NULL"));
> +        goto unlock;
> +    }
> +
> +    nodeDeviceUnlock();
> +    udevHandleOneDevice(device);
> +
> + cleanup:
> +    udev_device_unref(device);
> +    return;
> +
> + unlock:
> +    nodeDeviceUnlock();
> +    goto cleanup;
> +}
> +
> +
> +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 unlock;
> +    if (!udevCheckMonitorFD(udev_monitor, fd)) {
> +        nodeDeviceUnlock();
> +        return;
>      }

The above sequence will be done twice - once here and repeated again in
HandleThread. I understand why, but as I get to patch 3 - I'm not sure
things are going to work as expected.

John

> -
>      nodeDeviceUnlock();
> -    udevHandleOneDevice(device);
> -
> - cleanup:
> -    udev_device_unref(device);
> -    return;
>  
> - unlock:
> -    nodeDeviceUnlock();
> -    goto cleanup;
> +    udevEventHandleThread((void *)(intptr_t) fd);
>  }
>  
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/6] udev: Split udevEventHandleCallback in two functions
Posted by Erik Skultety 8 years, 2 months ago 
On Mon, Aug 28, 2017 at 11:04:35AM -0400, John Ferlan wrote:
>
>
> On 08/24/2017 07:23 AM, Erik Skultety wrote:
> > This patch splits udevEventHandleCallback in two (introduces
> > udevEventHandleThread) in order to be later able to refactor the latter
> > to actually become a detached thread which will wait some time for the
> > kernel to create the whole sysfs tree for a device as we cannot do that
> > in the event loop directly.
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 54 ++++++++++++++++++++++++++------------
> >  1 file changed, 37 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index 465d272ba..fe943c78b 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1647,36 +1647,56 @@ udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
> >
> >
> >  static void
> > +udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
>
> It's not ATTRIBUTE_UNUSED
>
> > +{
> > +    struct udev_device *device = NULL;
> > +    struct udev_monitor *udev_monitor = NULL;
> > +    int fd = (intptr_t) opaque;
>
> ^^^  opaque!

Right, good catch, thanks.

>
> > +
> > +    nodeDeviceLock();
> > +    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> > +
> > +    if (!udevCheckMonitorFD(udev_monitor, fd))
> > +        goto unlock;
> > +
> > +    device = udev_monitor_receive_device(udev_monitor);
> > +    if (device == NULL) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("udev_monitor_receive_device returned NULL"));
> > +        goto unlock;
> > +    }
> > +
> > +    nodeDeviceUnlock();
> > +    udevHandleOneDevice(device);
> > +
> > + cleanup:
> > +    udev_device_unref(device);
> > +    return;
> > +
> > + unlock:
> > +    nodeDeviceUnlock();
> > +    goto cleanup;
> > +}
> > +
> > +
> > +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 unlock;
> > +    if (!udevCheckMonitorFD(udev_monitor, fd)) {
> > +        nodeDeviceUnlock();
> > +        return;
> >      }
>
> The above sequence will be done twice - once here and repeated again in
> HandleThread. I understand why, but as I get to patch 3 - I'm not sure
> things are going to work as expected.

Okay, let's continue with the discussion in patch 3 comments :).

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.