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
