Re: [libvirt] [PATCH 3/5] nodedev: Clear the udev_monitor reference once unref'd
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(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
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(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.
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;
> }
> }
>
>