On Tue, Aug 01, 2017 at 03:25:37PM -0400, John Ferlan wrote:
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@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
Good point.
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@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.
I don't know, to me commenting such a change would appear as a bogus comment, IOW, you always have to protect access to shared data (unless implementing a lockless algorithm) so I think this change is self-explanatory, but anyways thanks for pointing it out, I'll definitely adjust the code. Erik
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; } }