Re: [libvirt PATCH v2 09/16] nodedev: add an mdevctl thread

On Tue, Aug 18, 2020 at 09:47:59AM -0500, Jonathon Jongsma wrote: > We need to peridocally query mdevctl for changes to device definitions > since an administrator can define new devices with mdevctl outside of > libvirt. > > In order to do this, a new thread is created to handle the querying. In > the future, mdevctl may add a way to signal device add / remove via > events, but for now we resort to a bit of a workaround: monitoring the > mdevctl config directories for changes to files. When a change is > detected, we query mdevctl and update our device list. > > Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; > --- > src/node_device/node_device_udev.c | 182 +++++++++++++++++++++++++++++ > 1 file changed, 182 insertions(+) > > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c > index e06584a3dc..a85418e549 100644 > --- a/src/node_device/node_device_udev.c > +++ b/src/node_device/node_device_udev.c > @@ -41,6 +41,7 @@ > #include "virnetdev.h" > #include "virmdev.h" > #include "virutil.h" > +#include <gio/gio.h> > > #include "configmake.h" > > @@ -66,6 +67,11 @@ struct _udevEventData { > virCond threadCond; > bool threadQuit; > bool dataReady; > + > + virThread mdevctlThread; > + virCond mdevctlCond; > + bool mdevctlReady; > + GList *mdevctl_monitors; > }; > > static virClassPtr udevEventDataClass; > @@ -85,8 +91,10 @@ udevEventDataDispose(void *obj) > udev = udev_monitor_get_udev(priv->udev_monitor); > udev_monitor_unref(priv->udev_monitor); > udev_unref(udev); > + g_list_free_full(priv->mdevctl_monitors, g_object_unref); > > virCondDestroy(&priv->threadCond); > + virCondDestroy(&priv->mdevctlCond); > } > > > @@ -117,6 +125,11 @@ udevEventDataNew(void) > return NULL; > } > > + if (virCondInit(&ret->mdevctlCond) < 0) { > + virObjectUnref(ret); > + return NULL; > + } > + > ret->watch = -1; > return ret; > } > @@ -1520,6 +1533,7 @@ nodeStateCleanup(void) > virObjectLock(priv); > priv->threadQuit = true; > virCondSignal(&priv->threadCond); > + virCondSignal(&priv->mdevctlCond); > virObjectUnlock(priv); > virThreadJoin(&priv->th); > } > @@ -1601,6 +1615,40 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv, > } > > > +/* Thread to query mdevctl for the current state of persistent mediated device > + * defintions when any changes are detected */ > +static > +void mdevctlThread(void *opaque G_GNUC_UNUSED) > +{ > + udevEventDataPtr priv = driver->privateData; > + > + while (1) { > + virObjectLock(priv); > + while (!priv->mdevctlReady && !priv->threadQuit) { > + if (virCondWait(&priv->mdevctlCond, &priv->parent.lock)) { > + virReportSystemError(errno, "%s", > + _("handler failed to wait on condition")); > + virObjectUnlock(priv); > + return; > + } > + } > + > + if (priv->threadQuit) { > + virObjectUnlock(priv); > + return; > + } > + > + virObjectUnlock(priv); > + > + mdevctlEnumerateDevices(); > + > + virObjectLock(priv); > + priv->mdevctlReady = false; > + virObjectUnlock(priv); > + } > +} > + > + > /** > * udevEventHandleThread > * @opaque: unused > @@ -1708,6 +1756,69 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, > } > > > +static int timeout_id; Please keep driver state in the driver state struct. > + > +static gboolean > +signalMdevctlThread(void *user_data) > +{ > + udevEventDataPtr priv = user_data; > + > + if (timeout_id) { > + g_source_remove(timeout_id); > + timeout_id = 0; > + } > + > + virObjectLock(priv); > + priv->mdevctlReady = true; > + virCondSignal(&priv->mdevctlCond); > + virObjectUnlock(priv); > + > + return G_SOURCE_REMOVE; > +} I don't see why we need to have a timer that then signals a persistent thread. Why cant this just spawn a throw-away thread to do the work and avoid all the conditional signaling complexity.