[libvirt] [PATCH v4 7/7] nodedev: Work around the uevent race by hooking up virFileWaitForAccess

[Erik Skultety]

On Wed, Sep 20, 2017 at 10:33:14AM -0400, John Ferlan wrote:
>
>
> On 09/18/2017 12:34 PM, Erik Skultety wrote:
> > If we find ourselves in the situation that the 'add' uevent has been
> > fired earlier than the sysfs tree for a device was created, we should
> > use the best-effort approach and give kernel some predetermined amount
> > of time, thus waiting for the attributes to be ready rather than
> > discarding the device from our device list forever. If those don't appear
> > in the given time frame, we need to move on, since libvirt can't wait
> > indefinitely.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index 70e15ffb8..2f63256a3 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1166,9 +1166,23 @@ udevProcessMediatedDevice(struct udev_device *dev,
> >      char *canonicalpath = NULL;
> >      virNodeDevCapMdevPtr data = &def->caps->data.mdev;
> >
> > -    if (virAsprintf(&linkpath, "%s/mdev_type", udev_device_get_syspath(dev)) < 0)
> > +    /* Because of a kernel uevent race, we might get the 'add' event prior to
> > +     * the sysfs tree being ready, so any attempt to access any sysfs attribute
> > +     * would result in ENOENT and us dropping the device, so let's work around
> > +     * it by waiting for the attributes to become available.
> > +     */
> > +
> > +    if (virAsprintf(&linkpath, "%s/mdev_type",
> > +                    udev_device_get_syspath(dev)) < 0)
> >          goto cleanup;
> >
> > +    if (virFileWaitForExists(linkpath, 1, 100) < 0) {
> > +        virReportSystemError(errno,
> > +                             _("failed to wait for file '%s' to appear"),
> > +                             linkpath);
> > +        goto cleanup;
> > +    }
> > +
>
> So the linkpath gets created after the canonicalpath... and we don't

Well, I would assume so, I believe that the kernel wouldn't create symlinks
first and the canonical paths second, such a design would be sooo broken.

Thanks,
Erik

> have to check that right?
>
> Considering what I pointed out in my previous review - I wouldn't be
> able to use virFileWaitForExists, but a similar loop would be possible.
> For NPIV the file exists, it's the contents that are bogus momentarily.
>
> Other consumers waiting that I looked at usually wait on some sort of
> open() and usleep() when ENOENT is returned (there's multiple examples
> if you search on ulseep).  Wonder if any of those could utilize
> something like this...  I know patches welcome ;-)
>
> Reviewed-by: John Ferlan <jferlan at redhat.com>
>
>
> >      if (virFileResolveLink(linkpath, &canonicalpath) < 0) {
> >          virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
> >          goto cleanup;
> >