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