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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > > > > if (virFileResolveLink(linkpath, &canonicalpath) < 0) { > > virReportSystemError(errno, _("failed to resolve '%s'"), linkpath); > > goto cleanup; > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
