On Fri, Apr 12, 2024 at 03:36 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:

From: Boris Fiuczynski <fiuczy@linux.ibm.com>

Two situations will trigger an udev add event: 1) the mdev is created when started (transient) or 2) the mdev was defined and is started In case 1 there is no node object existing and no config data is copied. In case 2 copying the active config data of an existing node object will only copy invalid data. Instead copying the defined config data will store valid data into the newly added node object.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>

This s-o-b is there by accident, will remove it in the next version.

--- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f1e402f8f7f6..4730a5b986ca 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device) objdef = virNodeDeviceObjGetDef(obj);

if (is_mdev) - nodeDeviceDefCopyFromMdevctl(def, objdef, false); + nodeDeviceDefCopyFromMdevctl(def, objdef, true);

persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj); -- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org -- Kind regards / Beste Grüße Marc Hartmayer

IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/12/24 15:36, Marc Hartmayer wrote:

@def is owned by @obj after adding it the node device object list. As soon as the @obj lock has been released, another thread could free @obj and therefore @def. If now someone accesses @def this would lead to a heap-use-after-free and therefore most likely to a segmentation fault, therefore set @def to NULL after the ownership has moved.

While at it, add comments to other code places why @def is set to NULL.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 4 ++++ src/test/test_driver.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4730a5b986ca..6613528d0e37 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1588,6 +1588,8 @@ udevAddOneDevice(struct udev_device *device) * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + /* @def is now owned by @obj */ + def = NULL; virNodeDeviceObjSetPersistent(obj, persistent); virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj); @@ -1983,6 +1985,8 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup;

+ /* @def is now owned by @obj */ + def = NULL; virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetAutostart(obj, true); virNodeDeviceObjSetPersistent(obj, true); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ed0cdc0dabe8..8972212e4ddb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7656,8 +7656,9 @@ testNodeDeviceMockCreateVport(testDriver *driver,

if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjSetSkipUpdateCaps(obj, true); + /* @def is now owned by @obj */ def = NULL; + virNodeDeviceObjSetSkipUpdateCaps(obj, true); objdef = virNodeDeviceObjGetDef(obj);

event = virNodeDeviceEventLifecycleNew(objdef->name,

-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

From: Boris Fiuczynski <fiuczy@linux.ibm.com>

When an udev add event occurs the mdev active config data requires an update via mdevctl as the udev does not contain all config data. This update needs to occur immediate and to be finished before the

s/immediate/immediately/

libvirt CREATE event is issued to keep the API usage reliable.

After this change, scheduleMdevctlUpdate call is already called in `udevAddOneDevice` and can therefore be removed in `udevHandleOneDevice`.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6613528d0e37..03ef0c14371a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1535,6 +1535,7 @@ udevSetParent(struct udev_device *device, static int udevAddOneDevice(struct udev_device *device) { + g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; virNodeDeviceObj *obj = NULL; virNodeDeviceDef *objdef; @@ -1549,6 +1550,9 @@ udevAddOneDevice(struct udev_device *device) def = g_new0(virNodeDeviceDef, 1);

def->sysfs_path = g_strdup(udev_device_get_syspath(device)); + /* Create a copy of sysfs_path so it can be safely accessed, even without + * holding the @obj lock during the VIR_WARN(...) call at the end. */ + sysfs_path = g_strdup(def->sysfs_path);

udevGetStringProperty(device, "DRIVER", &def->driver);

@@ -1608,6 +1612,13 @@ udevAddOneDevice(struct udev_device *device) if (has_mdev_types) scheduleMdevctlUpdate(driver->privateData, false);

+ /* The added mdev needs an immediate active config update before + * the event is issued to allow sane API usage. */

How about simply something like "so that full device information is available at the time that the 'created' event is emitted"

+ if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) { + VIR_WARN("Update of mediated device %s failed", + NULLSTR_EMPTY(sysfs_path)); + } + > ret = 0;

cleanup: @@ -1758,19 +1769,12 @@ nodeStateCleanup(void) static int udevHandleOneDevice(struct udev_device *device) { - virNodeDevCapType dev_cap_type; const char *action = udev_device_get_action(device);

VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));

- if (STREQ(action, "add") || STREQ(action, "change")) { - int ret = udevAddOneDevice(device); - if (ret == 0 && - udevGetDeviceType(device, &dev_cap_type) == 0 && - dev_cap_type == VIR_NODE_DEV_CAP_MDEV) - scheduleMdevctlUpdate(driver->privateData, false); - return ret; - } + if (STREQ(action, "add") || STREQ(action, "change")) + return udevAddOneDevice(device);

if (STREQ(action, "remove")) return udevRemoveOneDevice(device);

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On Thu, Apr 18, 2024 at 10:03 AM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

...

From: Boris Fiuczynski <fiuczy@linux.ibm.com>

When a mdev device is destroyed or stopped the udev remove event handling needs to reset the active config data of the node object representing a persisted mdev.

Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_driver.h | 3 +++ src/util/virmdev.h | 4 ++++ src/conf/node_device_conf.c | 10 ++-------- src/node_device/node_device_driver.c | 13 +++++++++++++ src/node_device/node_device_udev.c | 1 + src/util/virmdev.c | 20 ++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 7 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b3bc4b2e96ed..f195cfef9d49 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -197,3 +197,6 @@ int nodeDeviceUpdate(virNodeDevice *dev, const char *xmlDesc, unsigned int flags); + +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def); diff --git a/src/util/virmdev.h b/src/util/virmdev.h index 853041ac0619..e8e69040e5d4 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig { size_t nattributes; };

+void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config); +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree);

As far as I can tell, this Free function is not actually used anywhere, either in this patch or any following patches.

Yep, was just for completeness. Shall I remove it? Maybe it will be used in the future and if we have a …Clear function, I guess it’s always good to have a …Free function as well.

...

+ typedef struct _virMediatedDeviceType virMediatedDeviceType; struct _virMediatedDeviceType { char *id; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb72..897c67d6af91 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) g_free(data->sg.path); break; case VIR_NODE_DEV_CAP_MDEV: - g_free(data->mdev.defined_config.type); - g_free(data->mdev.active_config.type); g_free(data->mdev.uuid); - for (i = 0; i < data->mdev.defined_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]); - g_free(data->mdev.defined_config.attributes); - for (i = 0; i < data->mdev.active_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]); - g_free(data->mdev.active_config.attributes); + virMediatedDeviceConfigClear(&data->mdev.defined_config); + virMediatedDeviceConfigClear(&data->mdev.active_config); g_free(data->mdev.parent_addr); break; case VIR_NODE_DEV_CAP_CSS_DEV: diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d99b48138ebf..f623339dc973 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, }

+/* A mediated device definition contains data from mdevctl about the active + * device. When the device is deactivated the active configuration data needs + * to be removed. */ +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def) +{ + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return; + + virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config); +}

It doesn't feel necessary to introduce a short single-use function here. I think it can just be inlined.

Don’t have a strong opinion about this, @Boris? […snip…]

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks.

-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

Remove the timeout when the udevEventData is disposed, analogous to priv->watch.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 3297bdd8d7ef..1d3be28f8f08 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,6 +88,9 @@ udevEventDataDispose(void *obj) if (priv->watch != -1) virEventRemoveHandle(priv->watch);

+ if (priv->mdevctlTimeout > 0) + virEventRemoveTimeout(priv->mdevctlTimeout); + if (!priv->udev_monitor) return;

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

It is done a little differently everywhere in libvirt, but most common is to test for != -1.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1d3be28f8f08..469538a1395d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,7 +88,7 @@ udevEventDataDispose(void *obj) if (priv->watch != -1) virEventRemoveHandle(priv->watch);

- if (priv->mdevctlTimeout > 0) + if (priv->mdevctlTimeout != -1) virEventRemoveTimeout(priv->mdevctlTimeout);

if (!priv->udev_monitor) @@ -139,6 +139,7 @@ udevEventDataNew(void) return NULL; }

+ ret->mdevctlTimeout = -1; ret->watch = -1; return ret; } @@ -2086,7 +2087,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) udevEventData *priv = opaque; virThread thread;

- if (priv->mdevctlTimeout > 0) { + if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; } @@ -2196,7 +2197,7 @@ scheduleMdevctlUpdate(udevEventData *data, bool force) { if (!force) { - if (data->mdevctlTimeout > 0) + if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, data, NULL);

OK Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the locking mechanism and accidentally removed the comment with the reason why the lock is taken. Restore this comment and add a new comment about the lock.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 469538a1395d..5d474acdc2e0 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -72,8 +72,10 @@ struct _udevEventData { /* init thread */ virThread *initThread;

- GList *mdevctlMonitors; + /* Protects @mdevctlMonitors and must be taken when `mdevctl` command is + * called to make sure only one thread can query mdevctl at a time. */ virMutex mdevctlLock; + GList *mdevctlMonitors; int mdevctlTimeout; };

@@ -2074,6 +2076,7 @@ static void mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) { udevEventData *priv = driver->privateData; + /* ensure only a single thread can query mdevctl at a time */ VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);

if (nodeDeviceUpdateMediatedDevices() < 0)

Serializing mdevctl queries is not strictly necessary, and in fact is removed in a later patch in this series (14), so I think we can just drop this patch, tbh. I'm not sure that this mdevctlLock is even necessary. The udevEventData struct is already a lockable object, so I think we could simply get rid of this lock and use the object lock to protect the mdevctlMonitors variable if we wanted. Jonathon

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

The new names make it easier to understand the purpose of the data.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 48 +++++++++++++++--------------- 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 0c393b6233a4..a121ad99a676 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -63,11 +63,11 @@ struct _udevEventData { struct udev_monitor *udev_monitor; int watch;

- /* Thread data */ - virThread *th; - virCond threadCond; - bool threadQuit; - bool dataReady; + /* Udev thread data */ + virThread *udevThread; + virCond udevThreadCond; + bool udevThreadQuit; + bool udevDataReady;

/* init thread */ virThread *initThread; @@ -105,7 +105,7 @@ udevEventDataDispose(void *obj) } virMutexDestroy(&priv->mdevctlLock);

- virCondDestroy(&priv->threadCond); + virCondDestroy(&priv->udevThreadCond); }

@@ -131,7 +131,7 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL;

- if (virCondInit(&ret->threadCond) < 0) { + if (virCondInit(&ret->udevThreadCond) < 0) { virObjectUnref(ret); return NULL; } @@ -1747,16 +1747,16 @@ nodeStateCleanup(void) priv = driver->privateData; if (priv) { VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->threadQuit = true; - virCondSignal(&priv->threadCond); + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); } if (priv->initThread) { virThreadJoin(priv->initThread); g_clear_pointer(&priv->initThread, g_free); } - if (priv->th) { - virThreadJoin(priv->th); - g_clear_pointer(&priv->th, g_free); + if (priv->udevThread) { + virThreadJoin(priv->udevThread); + g_clear_pointer(&priv->udevThread, g_free); } }

@@ -1865,15 +1865,15 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) /* continue rather than break from the loop on non-fatal errors */ while (1) { VIR_WITH_OBJECT_LOCK_GUARD(priv) { - while (!priv->dataReady && !priv->threadQuit) { - if (virCondWait(&priv->threadCond, &priv->parent.lock)) { + while (!priv->udevDataReady && !priv->udevThreadQuit) { + if (virCondWait(&priv->udevThreadCond, &priv->parent.lock)) { virReportSystemError(errno, "%s", _("handler failed to wait on condition")); return; } }

- if (priv->threadQuit) + if (priv->udevThreadQuit) return;

errno = 0; @@ -1904,7 +1904,7 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) * after the udev_monitor_receive_device wouldn't help much * due to event mgmt and scheduler timing. */ VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->dataReady = false; + priv->udevDataReady = false; }

continue; @@ -1931,11 +1931,11 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, VIR_LOCK_GUARD lock = virObjectLockGuard(priv);

if (!udevEventMonitorSanityCheck(priv, fd)) - priv->threadQuit = true; + priv->udevThreadQuit = true; else - priv->dataReady = true; + priv->udevDataReady = true;

- virCondSignal(&priv->threadCond); + virCondSignal(&priv->udevThreadCond); }

@@ -2045,8 +2045,8 @@ nodeStateInitializeEnumerate(void *opaque) VIR_WITH_OBJECT_LOCK_GUARD(priv) { ignore_value(virEventRemoveHandle(priv->watch)); priv->watch = -1; - priv->threadQuit = true; - virCondSignal(&priv->threadCond); + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); }

goto cleanup; @@ -2344,12 +2344,12 @@ nodeStateInitialize(bool privileged, udev_monitor_set_receive_buffer_size(priv->udev_monitor, 128 * 1024 * 1024);

- priv->th = g_new0(virThread, 1); - if (virThreadCreateFull(priv->th, true, udevEventHandleThread, + priv->udevThread = g_new0(virThread, 1); + if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread, "udev-event", false, NULL) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); - g_clear_pointer(&priv->th, g_free); + g_clear_pointer(&priv->udevThread, g_free); goto unlock; }

Sure. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/12/24 15:36, Marc Hartmayer wrote:

Inline `udevRemoveOneDevice` as it's used only once.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a121ad99a676..672da8f5a19f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1490,16 +1490,6 @@ udevRemoveOneDeviceSysPath(const char *path) return 0; }

- -static int -udevRemoveOneDevice(struct udev_device *device) -{ - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(path); -} - - static int udevSetParent(struct udev_device *device, virNodeDeviceDef *def) @@ -1788,8 +1778,11 @@ udevHandleOneDevice(struct udev_device *device) if (STREQ(action, "add") || STREQ(action, "change")) return udevAddOneDevice(device);

- if (STREQ(action, "remove")) - return udevRemoveOneDevice(device); + if (STREQ(action, "remove")) { + const char *path = udev_device_get_syspath(device); + + return udevRemoveOneDeviceSysPath(path); + }

if (STREQ(action, "move")) { const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD");

-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

Use the proper driver functions for the node state shutdown preparation and wait. In the next patch, these functions will be extended.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 54 +++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 672da8f5a19f..cec7d837c43e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1736,18 +1736,8 @@ nodeStateCleanup(void)

priv = driver->privateData; if (priv) { - VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->udevThreadQuit = true; - virCondSignal(&priv->udevThreadCond); - } - if (priv->initThread) { - virThreadJoin(priv->initThread); - g_clear_pointer(&priv->initThread, g_free); - } - if (priv->udevThread) { - virThreadJoin(priv->udevThread); - g_clear_pointer(&priv->udevThread, g_free); - } + g_clear_pointer(&priv->initThread, g_free); + g_clear_pointer(&priv->udevThread, g_free); }

virObjectUnref(priv); @@ -2440,12 +2430,52 @@ static virConnectDriver udevConnectDriver = { .nodeDeviceDriver = &udevNodeDeviceDriver, };

+static int +nodeStateShutdownPrepare(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); + } + return 0; +} + +static int +nodeStateShutdownWait(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + if (priv->initThread) + virThreadJoin(priv->initThread); + + if (priv->udevThread) + virThreadJoin(priv->udevThread); + return 0; +}

static virStateDriver udevStateDriver = { .name = "udev", .stateInitialize = nodeStateInitialize, /* 0.7.3 */ .stateCleanup = nodeStateCleanup, /* 0.7.3 */ .stateReload = nodeStateReload, /* 0.7.3 */ + .stateShutdownPrepare = nodeStateShutdownPrepare, + .stateShutdownWait = nodeStateShutdownWait, };

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On 4/12/24 8:36 AM, Marc Hartmayer wrote:

Use a worker pool for processing the udev events and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and the libvirt nodedev event creation.

TODOs: + IMO, it's better practice for all functions called by the virThreadPool's worker thread to pass the driver via parameter and not global variables. Easier to test and cleaner.

I'm not opposed, but I think it should be a separate commit since it introduces a lot of churn that is unrelated to the thread pool.

+ how many worker threads should we have at maximum? + there are still TODO's in the code + improve error reporting + improve naming - e.g. rename more udevXXX functions?

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_driver.h | 2 +- src/node_device/node_device_driver.c | 6 +- src/node_device/node_device_udev.c | 295 +++++++++++++++++++-------- 3 files changed, 209 insertions(+), 94 deletions(-)

diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f195cfef9d49..2781ad136d68 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -147,7 +147,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring, bool defined);

int -nodeDeviceUpdateMediatedDevices(void); +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *driver);

void nodeDeviceGenerateName(virNodeDeviceDef *def, diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index f623339dc973..59c5f9b417a4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1887,7 +1887,7 @@ removeMissingPersistentMdev(virNodeDeviceObj *obj,

int -nodeDeviceUpdateMediatedDevices(void) +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver) { g_autofree virNodeDeviceDef **defs = NULL; g_autofree virNodeDeviceDef **act_defs = NULL; @@ -1911,7 +1911,7 @@ nodeDeviceUpdateMediatedDevices(void) /* Any mdevs that were previously defined but were not returned in the * latest mdevctl query should be removed from the device list */ data.defs = defs; - virNodeDeviceObjListForEachRemove(driver->devs, + virNodeDeviceObjListForEachRemove(node_driver->devs, removeMissingPersistentMdev, &data);

for (i = 0; i < data.ndefs; i++) @@ -2374,7 +2374,7 @@ nodeDeviceUpdate(virNodeDevice *device, cleanup: virNodeDeviceObjEndAPI(&obj); if (updated) - nodeDeviceUpdateMediatedDevices(); + nodeDeviceUpdateMediatedDevices(driver);

return ret; }

So far, all of the changes have only been related to the change of the global driver variable.

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index cec7d837c43e..2a252d8fe62b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -43,6 +43,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include "virthreadpool.h"

#include "configmake.h"

@@ -69,14 +70,14 @@ struct _udevEventData { bool udevThreadQuit; bool udevDataReady;

- /* init thread */ - virThread *initThread; - /* Protects @mdevctlMonitors and must be taken when `mdevctl` command is * called to make sure only one thread can query mdevctl at a time. */ virMutex mdevctlLock; GList *mdevctlMonitors; int mdevctlTimeout; + + /* Immutable pointer, self-locking APIs */ + virThreadPool *workerPool; };

static virClass *udevEventDataClass; @@ -146,6 +147,79 @@ udevEventDataNew(void) return ret; }

+typedef enum { + NODE_DEVICE_EVENT_INIT = 0, + NODE_DEVICE_EVENT_ADD, + NODE_DEVICE_EVENT_REMOVE, + NODE_DEVICE_EVENT_CHANGE, + NODE_DEVICE_EVENT_MOVE,

These events are from the udev subsystem, so I think it would be nicer to name them such. NODE_DEVICE_EVENT_UDEV_ADD, etc.

+ NODE_DEVICE_EVENT_UPDATE,

And this isn't really an event -- it's a description of what you want to do in response to the event. It might make more sense to be named something like: NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED I don't mind too much either way.

+ + NODE_DEVICE_EVENT_LAST +} nodeDeviceEventType; + +struct _nodeDeviceEvent { + nodeDeviceEventType eventType; + void *data; +};

I think it might be nicer to have the caller decide how the data should be freed by passing a free function like so: struct _nodeDeviceEvent { nodeDeviceEventType eventType; void *data; virFreeCallback dataFree; }; typedef struct _nodeDeviceEvent nodeDeviceEvent; static nodeDeviceEvent* nodeDeviceEventNew(nodeDeviceEventType event_type, gpointer data, virFreeCallback dataFree) { nodeDeviceEvent *e = g_new0(nodeDeviceEvent, 1); e->eventType = event_type; e->data = data; e->dataFree = dataFree; return e; } ...

+typedef struct _nodeDeviceEvent nodeDeviceEvent; + +static void +nodeDeviceEventFree(nodeDeviceEvent *event) +{ + if (!event) + return; + + switch (event->eventType) { + case NODE_DEVICE_EVENT_INIT: + udev_unref(event->data); + break; + case NODE_DEVICE_EVENT_ADD: + case NODE_DEVICE_EVENT_CHANGE: + case NODE_DEVICE_EVENT_MOVE: + case NODE_DEVICE_EVENT_REMOVE: + udev_device_unref(event->data); + break; + case NODE_DEVICE_EVENT_UPDATE: + break; + case NODE_DEVICE_EVENT_LAST: + // TODO Bug! + g_abort();

Then you don't have any decisions to make here about what to do for different event types, you just do something like: if (event->data && event->dataFree) event->dataFree(event->data);

+ break; + } + g_free(event); +} > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(nodeDeviceEvent, nodeDeviceEventFree); + + /** + * nodeDeviceEventSubmit: + * @eventType: the event to be processed + * @data: additional data for the event processor (the pointer is stolen and it + * will be properly freed) + * + * Submits @eventType to be processed by the asynchronous event handling + * thread. + */ +static int nodeDeviceEventSubmit(nodeDeviceEventType eventType, void *data) +{ + nodeDeviceEvent *event = g_new0(nodeDeviceEvent, 1); + udevEventData *priv = NULL; + + /* BUG */ + if (!driver) + g_abort(); + + priv = driver->privateData; + + event->eventType = eventType; + event->data = data; + if (virThreadPoolSendJob(priv->workerPool, 0, event) < 0) { + nodeDeviceEventFree(event); + return -1; + } + return 0; +} +

static bool udevHasDeviceProperty(struct udev_device *dev, @@ -364,7 +438,7 @@ udevTranslatePCIIds(unsigned int vendor,

static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def) { virNodeDevCapPCIDev *pci_dev = &def->caps->data.pci_dev; @@ -375,8 +449,8 @@ udevProcessPCI(struct udev_device *device, char *p; bool privileged = false;

- VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - privileged = driver->privileged; + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + privileged = driver_state->privileged; }

pci_dev->klass = -1; @@ -1394,12 +1468,12 @@ udevGetDeviceType(struct udev_device *device,

static int -udevGetDeviceDetails(struct udev_device *device, +udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def) { switch (def->caps->data.type) { case VIR_NODE_DEV_CAP_PCI_DEV: - return udevProcessPCI(device, def); + return udevProcessPCI(driver_state, device, def); case VIR_NODE_DEV_CAP_USB_DEV: return udevProcessUSBDevice(device, def); case VIR_NODE_DEV_CAP_USB_INTERFACE: @@ -1450,13 +1524,13 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force);

static int -udevRemoveOneDeviceSysPath(const char *path) +processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path) { virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; virObjectEvent *event = NULL;

- if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) { + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, path))) { VIR_DEBUG("Failed to find device to remove that has udev path '%s'", path); return -1; @@ -1477,21 +1551,21 @@ udevRemoveOneDeviceSysPath(const char *path) } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver_state->devs, obj); } virNodeDeviceObjEndAPI(&obj);

/* cannot check for mdev_types since they have already been removed */ VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + scheduleMdevctlUpdate(driver_state->privateData, false); }

- virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; }

static int -udevSetParent(struct udev_device *device, +udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def) { struct udev_device *parent_device = NULL; @@ -1514,7 +1588,7 @@ udevSetParent(struct udev_device *device, return -1; }

- if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + if ((obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); def->parent = g_strdup(objdef->name); @@ -1532,7 +1606,7 @@ udevSetParent(struct udev_device *device, }

static int -udevAddOneDevice(struct udev_device *device) +processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struct udev_device *device) { g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1563,15 +1637,15 @@ udevAddOneDevice(struct udev_device *device) if (udevGetDeviceNodes(device, def) != 0) goto cleanup;

- if (udevGetDeviceDetails(device, def) != 0) + if (udevGetDeviceDetails(driver_state, device, def) != 0) goto cleanup;

- if (udevSetParent(device, def) != 0) + if (udevSetParent(driver_state, device, def) != 0) goto cleanup;

is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV;

- if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(driver_state->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj);

if (is_mdev) @@ -1589,7 +1663,7 @@ udevAddOneDevice(struct udev_device *device)

/* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver_state->devs, def))) goto cleanup; /* @def is now owned by @obj */ def = NULL; @@ -1609,14 +1683,14 @@ udevAddOneDevice(struct udev_device *device) virNodeDeviceObjEndAPI(&obj);

if (has_mdev_types) { - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) { + scheduleMdevctlUpdate(driver_state->privateData, false); } }

/* The added mdev needs an immediate active config update before * the event is issued to allow sane API usage. */ - if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) { + if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } @@ -1624,7 +1698,7 @@ udevAddOneDevice(struct udev_device *device) ret = 0;

cleanup: - virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event);

if (ret != 0) { VIR_DEBUG("Discarding device %d %p %s", ret, def, @@ -1637,7 +1711,7 @@ udevAddOneDevice(struct udev_device *device)

static int -udevProcessDeviceListEntry(struct udev *udev, +udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev *udev, struct udev_list_entry *list_entry) { struct udev_device *device; @@ -1649,7 +1723,7 @@ udevProcessDeviceListEntry(struct udev *udev, device = udev_device_new_from_syspath(udev, name);

if (device != NULL) { - if (udevAddOneDevice(device) != 0) { + if (processNodeDeviceAddAndChangeEvent(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1687,7 +1761,7 @@ udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate)

static int -udevEnumerateDevices(struct udev *udev) +udevEnumerateDevices(virNodeDeviceDriverState *driver_state, struct udev *udev) { struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; @@ -1703,7 +1777,7 @@ udevEnumerateDevices(struct udev *udev) udev_list_entry_foreach(list_entry, udev_enumerate_get_list_entry(udev_enumerate)) {

- udevProcessDeviceListEntry(udev, list_entry); + udevProcessDeviceListEntry(driver_state, udev, list_entry); }

ret = 0; @@ -1736,8 +1810,8 @@ nodeStateCleanup(void)

priv = driver->privateData; if (priv) { - g_clear_pointer(&priv->initThread, g_free); g_clear_pointer(&priv->udevThread, g_free); + virThreadPoolFree(priv->workerPool); }

virObjectUnref(priv); @@ -1765,26 +1839,19 @@ udevHandleOneDevice(struct udev_device *device)

VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));

- if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); - - if (STREQ(action, "remove")) { - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(path); - } - - if (STREQ(action, "move")) { - const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); - - if (devpath_old) { - g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); - - udevRemoveOneDeviceSysPath(devpath_old_fixed); - } - - return udevAddOneDevice(device); + /* Reference is either released via workerpool logic or at the end of this + * function. */ + device = udev_device_ref(device); + if (STREQ(action, "add")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_ADD, device); + } else if (STREQ(action, "change")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_CHANGE, device); + } else if (STREQ(action, "remove")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_REMOVE, device); + } else if (STREQ(action, "move")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_MOVE, device); } + udev_device_unref(device);

return 0; } @@ -2003,23 +2070,22 @@ udevSetupSystemDev(void)

static void -nodeStateInitializeEnumerate(void *opaque) +processNodeStateInitializeEnumerate(virNodeDeviceDriverState *event_driver, struct udev *udev) { - struct udev *udev = opaque; - udevEventData *priv = driver->privateData; + udevEventData *priv = event_driver->privateData;

/* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) + if (udevEnumerateDevices(event_driver, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices() != 0) + if (nodeDeviceUpdateMediatedDevices(event_driver) != 0) goto error;

cleanup: - VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - driver->initialized = true; - virCondBroadcast(&driver->initCond); + VIR_WITH_MUTEX_LOCK_GUARD(&event_driver->lock) { + event_driver->initialized = true; + virCondBroadcast(&event_driver->initCond); }

return; @@ -2059,35 +2125,17 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) return 0; }

- static void -mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) -{ - udevEventData *priv = driver->privateData; - /* ensure only a single thread can query mdevctl at a time */ - VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); - - if (nodeDeviceUpdateMediatedDevices() < 0) - VIR_WARN("mdevctl failed to update mediated devices"); -} - - -static void -launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) +submitMdevctlUpdate(int timer G_GNUC_UNUSED, void *opaque) { udevEventData *priv = opaque; - virThread thread;

if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; }

- if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, NULL) < 0) { - virReportSystemError(errno, "%s", - _("failed to create mdevctl thread")); - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UPDATE, NULL); }

@@ -2182,7 +2230,7 @@ mdevctlEnableMonitor(udevEventData *priv) /* Schedules an mdevctl update for 100ms in the future, canceling any existing * timeout that may have been set. In this way, multiple update requests in * quick succession can be collapsed into a single update. if @force is true, - * an update thread will be spawned immediately. */ + * the worker job is submitted immediately. */ static void scheduleMdevctlUpdate(udevEventData *data, bool force) @@ -2190,12 +2238,12 @@ scheduleMdevctlUpdate(udevEventData *data, if (!force) { if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); return; }

- launchMdevctlUpdateThread(-1, data); + submitMdevctlUpdate(-1, data); }

@@ -2235,6 +2283,67 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, }

+static void nodeDeviceEventHandler(void *data, void *opaque) +{ + virNodeDeviceDriverState *driver_state = opaque; + g_autoptr(nodeDeviceEvent) processEvent = data; + + switch (processEvent->eventType) { + case NODE_DEVICE_EVENT_INIT: + { + struct udev *udev = processEvent->data; + + processNodeStateInitializeEnumerate(driver_state, udev); + } + break; + case NODE_DEVICE_EVENT_ADD: + case NODE_DEVICE_EVENT_CHANGE: + { + struct udev_device *device = processEvent->data; + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_REMOVE: + { + struct udev_device *device = processEvent->data; + const char *path = udev_device_get_syspath(device); + + processNodeDeviceRemoveEvent(driver_state, path); + } + break; + case NODE_DEVICE_EVENT_MOVE: + { + struct udev_device *device = processEvent->data; + const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); + + if (devpath_old) { + g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); + + processNodeDeviceRemoveEvent(driver_state, devpath_old_fixed); + } + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_UPDATE: + { + udevEventData *priv = driver_state->privateData; + /* ensure only a single thread can query mdevctl at a time */ + VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); + + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); + } + break; + case NODE_DEVICE_EVENT_LAST: + // TODO Bug! + g_abort(); + break; + } +} + + static int nodeStateInitialize(bool privileged, const char *root, @@ -2301,6 +2410,16 @@ nodeStateInitialize(bool privileged, driver->parserCallbacks.postParse = nodeDeviceDefPostParse; driver->parserCallbacks.validate = nodeDeviceDefValidate;

+ /* must be initialized before trying to reconnect to all the running mdevs + * since there might occur some mdevctl monitor events that will be + * dispatched to the worker pool */ + priv->workerPool = virThreadPoolNewFull(1, 10, 0, nodeDeviceEventHandler, + "nodev-device-event", + NULL, + driver); + if (!priv->workerPool) + goto unlock; + if (udevPCITranslateInit(privileged) < 0) goto unlock;

@@ -2359,14 +2478,7 @@ nodeStateInitialize(bool privileged, if (udevSetupSystemDev() != 0) goto cleanup;

- priv->initThread = g_new0(virThread, 1); - if (virThreadCreateFull(priv->initThread, true, nodeStateInitializeEnumerate, - "nodedev-init", false, udev) < 0) { - virReportSystemError(errno, "%s", - _("failed to create udev enumerate thread")); - g_clear_pointer(&priv->initThread, g_free); - goto cleanup; - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_INIT, udev_ref(udev));

return VIR_DRV_STATE_INIT_COMPLETE;

@@ -2446,6 +2558,9 @@ nodeStateShutdownPrepare(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } + + if (priv->workerPool) + virThreadPoolStop(priv->workerPool); return 0; }

@@ -2461,11 +2576,11 @@ nodeStateShutdownWait(void) if (!priv) return 0;

- if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) virThreadJoin(priv->udevThread); + + if (priv->workerPool) + virThreadPoolDrain(priv->workerPool); return 0; }