[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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;
> >          }
> >      }
> >
> >


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]