[libvirt] [PATCH] nodedev: Fix double unlock of the driver on udevEnumerateDevices failure

[Erik Skultety]

On Fri, Jul 28, 2017 at 11:31:27AM +0200, Michal Privoznik wrote:
> On 07/26/2017 10:45 AM, Erik Skultety wrote:
> > Commit @4cb719b2dc moved the driver locks around since these have become
> > unnecessary at spots where the code handles now self-lockable object
> > list, but missed the possible double unlock if udevEnumerateDevices
> > fails, because at that point the driver lock had been already dropped.
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
>
> I know you already pushed this but what's the point of node driver lock
> anyway? There are only two places where the driver lock is used - init

To answer the question: to protect the access to driver's private data which
are mutable. Once we introduce an event handler thread for udev events, you'll
get concurrent access to the udev monitor. As you say, the driver's lock
doesn't serve much purpose except for accessing the udev monitor. This wasn't
discussed on a mailing list unfortunately, but I asked about the need to
perform any kinds of sanity checks on the udev_monitor on IRC, because noone
can change that one except ourselves - let's say we dropped the sanity checks,
then there's basically no need to have the driver lock (except for maybe
consistency among other drivers) at all. But I recall some other opinions in
favour of keeping the sanity checks, don't remember the specifics though.

Erik

> and cleanup. Moreover with the current code still racy, except not
> really beacuse init and cleanup will never be called concurrently.
> Therefore the lock can be dropped entirely.
>
> Michal