Re: [libvirt] [PATCH 3/5] nodedev: Clear the udev_monitor reference once unref'd
On 07/26/2017 02:44 AM, Erik Skultety wrote:
Since we only have one udev_monitor reference throughout libvirt, we
should either clear the pointer we copy around to prevent invalid memory
access once we unref this single reference, or start using reference
counting provided by libudev. This patch follows the former and only
clears the pointer.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/node_device/node_device_udev.c | 1 +
1 file changed, 1 insertion(+)
What's here, works, but expanding things out a bit for this one...
Since we have the nodeDeviceLock, why not within:
if (priv) {
}
do a VIR_FREE(driver->privateData); inside the if, that should set
driver->privateData = NULL too. Avoids something finding driver->priv
later in the instant that the nodeDeviceUnlock is run and something else
gets the lock and looks @priv... IOW: See, patch 5 comments.
I think if driver->priv is set to NULL, then there'd be no need to set
udev_monitor to NULL, but it cannot hurt especially if this is some sort
of race with udevEventHandleCallback, so...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
But I do think the VIR_FREE(priv) should move and be a
VIR_FREE(driver->privateData)... Note, not @priv because that won't set
@privateData = NULl which is what we'd want. You may even want to leave
a comment about that indicating trying to close a race w/ event handling.
John
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index ea10dc3ce..cd19e79c1 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1581,6 +1581,7 @@ nodeStateCleanup(void)
if (udev_monitor != NULL) {
udev = udev_monitor_get_udev(udev_monitor);
udev_monitor_unref(udev_monitor);
+ priv->udev_monitor = NULL;
}
}