This patch splits udevEventHandleCallback in two (introduces
udevEventHandleThread) in order to be later able to refactor the latter
to actually become a normal 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 | 41 +++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index bb9787fdb..f7646cd8a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1691,32 +1691,49 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
 
 
 static void
+udevEventHandleThread(void *opaque)
+{
+    udevEventDataPtr priv = driver->privateData;
+    int fd = (intptr_t) opaque;
+    struct udev_device *device = NULL;
+
+    virObjectLock(priv);
+
+    if (!udevEventMonitorSanityCheck(priv, fd)) {
+        virObjectUnlock(priv);
+        return;
+    }
+
+    device = udev_monitor_receive_device(priv->udev_monitor);
+    virObjectUnlock(priv);
+
+    if (device == NULL) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("udev_monitor_receive_device returned NULL"));
+        return;
+    }
+
+    udevHandleOneDevice(device);
+    udev_device_unref(device);
+}
+
+
+static void
 udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
                         int fd,
                         int events ATTRIBUTE_UNUSED,
                         void *data ATTRIBUTE_UNUSED)
 {
     udevEventDataPtr priv = driver->privateData;
-    struct udev_device *device = NULL;
 
     virObjectLock(priv);
-
     if (!udevEventMonitorSanityCheck(priv, fd)) {
         virObjectUnlock(priv);
         return;
     }
-
-    device = udev_monitor_receive_device(priv->udev_monitor);
     virObjectUnlock(priv);
 
-    if (device == NULL) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("udev_monitor_receive_device returned NULL"));
-        return;
-    }
-
-    udevHandleOneDevice(device);
-    udev_device_unref(device);
+    udevEventHandleThread((void *)(intptr_t) fd);
 }
 
 
-- 
2.13.6

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

On 10/11/2017 10:52 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 normal 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 | 41 +++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index bb9787fdb..f7646cd8a 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1691,32 +1691,49 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
>  
>  
>  static void
> +udevEventHandleThread(void *opaque)
> +{
> +    udevEventDataPtr priv = driver->privateData;
> +    int fd = (intptr_t) opaque;
> +    struct udev_device *device = NULL;
> +

To go along with my thought from patch 2, should there be a "if (!priv)"
check here too?  I believe it doesn't matter yet because there are
driver level locks that would seemingly prevent the thread code from
running if Cleanup occurs, but soon enough that won't be the case.

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

John

> +    virObjectLock(priv);
> +
> +    if (!udevEventMonitorSanityCheck(priv, fd)) {
> +        virObjectUnlock(priv);
> +        return;
> +    }
> +
> +    device = udev_monitor_receive_device(priv->udev_monitor);
> +    virObjectUnlock(priv);
> +
> +    if (device == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("udev_monitor_receive_device returned NULL"));
> +        return;
> +    }
> +
> +    udevHandleOneDevice(device);
> +    udev_device_unref(device);
> +}
> +
> +
> +static void
>  udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>                          int fd,
>                          int events ATTRIBUTE_UNUSED,
>                          void *data ATTRIBUTE_UNUSED)
>  {
>      udevEventDataPtr priv = driver->privateData;
> -    struct udev_device *device = NULL;
>  
>      virObjectLock(priv);
> -
>      if (!udevEventMonitorSanityCheck(priv, fd)) {
>          virObjectUnlock(priv);
>          return;
>      }
> -
> -    device = udev_monitor_receive_device(priv->udev_monitor);
>      virObjectUnlock(priv);
>  
> -    if (device == NULL) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("udev_monitor_receive_device returned NULL"));
> -        return;
> -    }
> -
> -    udevHandleOneDevice(device);
> -    udev_device_unref(device);
> +    udevEventHandleThread((void *)(intptr_t) fd);
>  }
>  
>  
> 

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

On Sun, Oct 15, 2017 at 10:23:52AM -0400, John Ferlan wrote:
>
>
> On 10/11/2017 10:52 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 normal 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 | 41 +++++++++++++++++++++++++++-----------
> >  1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index bb9787fdb..f7646cd8a 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1691,32 +1691,49 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
> >
> >
> >  static void
> > +udevEventHandleThread(void *opaque)
> > +{
> > +    udevEventDataPtr priv = driver->privateData;
> > +    int fd = (intptr_t) opaque;
> > +    struct udev_device *device = NULL;
> > +
>
> To go along with my thought from patch 2, should there be a "if (!priv)"
> check here too?  I believe it doesn't matter yet because there are
> driver level locks that would seemingly prevent the thread code from
> running if Cleanup occurs, but soon enough that won't be the case.

It's called from within Initialize which is the main thread, if !priv,
Initialize would have failed by this point

Erik

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