On 13. 3. 2020 13:00, Daniel P. Berrangé wrote: > During startup the udev node device driver impl uses a background thread > to populate the list of devices to avoid blocking the daemon startup > entirely. There is no synchronization to the public APIs, so it is > possible for an application to start calling APIs before the device > initialization is complete. > > This was not a problem in the old approach where libvirtd was started > on boot, as initialization would easily complete before any APIs were > called. > > With the use of socket activation, however, APIs are invoked from the > very moment the daemon starts. This is easily seen by doing a > > 'virsh -c nodedev:///system list' > > the first time it runs it will only show one or two devices. The second > time it runs it will show all devices. The solution is to introduce a > flag and condition variable for APIs to synchronize against before > returning any data. > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com> > --- > src/conf/virnodedeviceobj.h | 2 ++ > src/node_device/node_device_driver.c | 44 ++++++++++++++++++++++++++++ > src/node_device/node_device_hal.c | 15 ++++++++++ > src/node_device/node_device_udev.c | 13 ++++++++ > 4 files changed, 74 insertions(+) >
For both drivers: > @@ -1744,6 +1745,11 @@ nodeStateInitializeEnumerate(void *opaque) > if (udevEnumerateDevices(udev) != 0) > goto error; > > + nodeDeviceLock(); > + driver->initialized = true; > + nodeDeviceUnlock(); > + virCondBroadcast(&driver->initCond); Should this broadcast be in the critical section? Just asking, for this simple case it may be okay. > + > return; > > error: > @@ -1806,6 +1812,13 @@ nodeStateInitialize(bool privileged, > VIR_FREE(driver); > return VIR_DRV_STATE_INIT_ERROR; > } > + if (virCondInit(&driver->initCond) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Unable to initialize condition variable")); And perhaps, virReportSystemError() would be nicer. > + virMutexDestroy(&driver->lock); > + VIR_FREE(driver); > + return VIR_DRV_STATE_INIT_ERROR; > + } > > driver->privileged = privileged; > > Reviewed-by: Michal Privoznik <mpriv...@redhat.com> Michal