Re: [libvirt] [PATCH v5 4/6] udev: Convert udevEventHandleThread to an actual thread routine

Adjust udevEventHandleThread to be a proper thread routine running in an infinite loop handling devices. The handler thread pulls all available data from the udev monitor and only then waits until a wakeup signal for new incoming data has been emitted by udevEventHandleCallback.

Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 125 +++++++++++++++++++++++++++---------- 1 file changed, 93 insertions(+), 32 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f7646cd8a..a6d7e6d70 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -62,6 +62,12 @@ struct _udevEventData { struct udev_monitor *udev_monitor; int watch; bool privileged; + + /* Thread data */ + virThread th; + virCond threadCond; + bool threadQuit; + bool dataReady; }; static virClassPtr udevEventDataClass; @@ -80,6 +86,8 @@ udevEventDataDispose(void *obj) udev_unref(udev); udev_monitor_unref(data->udev_monitor); + + virCondDestroy(&data->threadCond); } @@ -108,9 +116,15 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL; + if (virCondInit(&ret->threadCond) < 0) { + virObjectUnref(ret); + return NULL; + } + ret->watch = -1; return ret; -} +}; +

static bool @@ -1625,15 +1639,25 @@ udevPCITranslateDeinit(void) static int nodeStateCleanup(void) { + udevEventDataPtr priv = NULL; + if (!driver) return -1; + priv = driver->privateData; +

nodeDeviceLock(); - virObjectUnref(driver->privateData); + virObjectLock(priv); + priv->threadQuit = true; + virCondSignal(&priv->threadCond); + virObjectUnlock(priv); + virThreadJoin(&priv->th); + + virObjectUnref(priv); virObjectUnref(driver->nodeDeviceEventState); - virNodeDeviceObjListFree(driver->devs); + nodeDeviceUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -1691,30 +1715,60 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv, static void -udevEventHandleThread(void *opaque) +udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED) { udevEventDataPtr priv = driver->privateData; - int fd = (intptr_t) opaque; struct udev_device *device = NULL; - virObjectLock(priv); + /* continue rather than break from the loop on non-fatal errors */ + while (1) { + virObjectLock(priv); + while (!priv->dataReady && !priv->threadQuit) { + if (virCondWait(&priv->threadCond, &priv->parent.lock)) { + virReportSystemError(errno, "%s", + _("handler failed to wait on condition")); + virObjectUnlock(priv); + return; + } + } - if (!udevEventMonitorSanityCheck(priv, fd)) { + if (priv->threadQuit) { + virObjectUnlock(priv); + return; + } + + errno = 0; + device = udev_monitor_receive_device(priv->udev_monitor); virObjectUnlock(priv); - return; - } - device = udev_monitor_receive_device(priv->udev_monitor); - virObjectUnlock(priv); + if (!device) { + if (errno == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_receive_device failed")); + return; + } - if (device == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("udev_monitor_receive_device returned NULL")) - return - } + /* POSIX allows both EAGAIN and EWOULDBLOCK to be used + * interchangeably when the read would block or timeout was fired + */ + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR + if (errno != EAGAIN && errno != EWOULDBLOCK) { + VIR_WARNINGS_RESET + virReportSystemError(errno, "%s", + _("udev_monitor_receive_device failed"));

+ return; + } + + virObjectLock(priv); + priv->dataReady = false; + virObjectUnlock(priv); - udevHandleOneDevice(device); - udev_device_unref(device); + continue; + } + + udevHandleOneDevice(device); + udev_device_unref(device); + } } @@ -1722,18 +1776,19 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED) { udevEventDataPtr priv = driver->privateData; virObjectLock(priv); - if (!udevEventMonitorSanityCheck(priv, fd)) { - virObjectUnlock(priv); - return; - } + + if (!udevEventMonitorSanityCheck(priv, fd)) + priv->threadQuit = true; + else + priv->dataReady = true; + + virCondSignal(&priv->threadCond); virObjectUnlock(priv); - - udevEventHandleThread((void *)(intptr_t) fd); } @@ -1875,29 +1930,29 @@ nodeStateInitialize(bool privileged, return -1; } - nodeDeviceLock(); -

if (!(driver->devs = virNodeDeviceObjListNew()) || !(priv = udevEventDataNew())) - goto unlock; + goto cleanup; driver->privateData = priv; driver->nodeDeviceEventState = virObjectEventStateNew(); if (udevPCITranslateInit(privileged) < 0) - goto unlock; + goto cleanup; udev = udev_new(); if (!udev) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create udev context")); - goto unlock; + goto cleanup; } #if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction); #endif + virObjectLock(priv); + priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (!priv->udev_monitor) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1916,6 +1971,12 @@ nodeStateInitialize(bool privileged, 128 * 1024 * 1024); #endif + if (virThreadCreate(&priv->th, true, udevEventHandleThread, NULL) < 0) { + virReportSystemError(errno, "%s", + _("failed to create udev handler thread")); + goto unlock; + } + /* We register the monitor with the event callback so we are * notified by udev of device changes before we enumerate existing * devices because libvirt will simply recreate the device if we @@ -1934,7 +1995,7 @@ nodeStateInitialize(bool privileged, if (udevSetupSystemDev() != 0) goto unlock; - nodeDeviceUnlock(); + virObjectUnlock(priv); /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) @@ -1947,7 +2008,7 @@ nodeStateInitialize(bool privileged, return -1; unlock: - nodeDeviceUnlock(); + virObjectUnlock(priv); goto cleanup; }