[libvirt] [PATCH 2/3] util: Introduce virFileWaitForAccess

[Erik Skultety]

On Tue, Jun 20, 2017 at 12:21:48PM -0400, Dawid Zamirski wrote:
> On Tue, 2017-06-20 at 17:45 +0200, Michal Privoznik wrote:
> > On 06/20/2017 05:21 PM, Dawid Zamirski wrote:
> > > On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote:
> > > > Since we have a number of places where we workaround timing
> > > > issues
> > > > with
> > > > devices, attributes (files in general) not being available at the
> > > > time
> > > > of processing them by calling usleep in a loop for a fixed number
> > > > of
> > > > tries, we could as well have a utility function that would do
> > > > that.
> > > > Therefore we won't have to duplicate this ugly workaround even
> > > > more.
> > > >
> > > > This is a prerequisite for
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
> > > >
> > > > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > > > ---
> > > >  src/libvirt_private.syms |  1 +
> > > >  src/util/virfile.c       | 36
> > > > ++++++++++++++++++++++++++++++++++++
> > > >  src/util/virfile.h       |  2 ++
> > > >  3 files changed, 39 insertions(+)
> > > >
> > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > > index c1e9471c5..53878a30f 100644
> > > > --- a/src/libvirt_private.syms
> > > > +++ b/src/libvirt_private.syms
> > > > @@ -1698,6 +1698,7 @@ virFileStripSuffix;
> > > >  virFileTouch;
> > > >  virFileUnlock;
> > > >  virFileUpdatePerm;
> > > > +virFileWaitForAccess;
> > > >  virFileWrapperFdClose;
> > > >  virFileWrapperFdFree;
> > > >  virFileWrapperFdNew;
> > > > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > > > index 6bbcc3d15..0b1a91699 100644
> > > > --- a/src/util/virfile.c
> > > > +++ b/src/util/virfile.c
> > > > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const
> > > > char *format, ...)
> > > >      VIR_FREE(str);
> > > >      return ret;
> > > >  }
> > > > +
> > > > +
> > > > +/**
> > > > + * virFileWaitForAccess:
> > > > + * @path: absolute path to a sysfs attribute (can be a symlink)
> > > > + * @ms: how long to wait (in milliseconds)
> > > > + * @tries: how many times should we try to wait for @path to
> > > > become
> > > > accessible
> > > > + *
> > > > + * Checks the existence of @path. In case the file defined by
> > > > @path
> > > > + * doesn't exist, we wait for it to appear in 100ms (for up to
> > > > @tries times).
> > > > + *
> > > > + * Returns 0 on success, -1 on error (ENOENT is fine here).
> > > > + */
> > > > +int
> > > > +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> > > > +{
> > > > +    errno = 0;
> > > > +
> > > > +    /* wait for @path to be accessible in @ms milliseconds, up
> > > > to
> > > > @tries */
> > > > +    while (tries-- > 0 && !virFileExists(path)) {
> > > > +        if (errno != ENOENT) {
> > > > +            virReportSystemError(errno, "%s", path);
> > > > +            return -1;
> > > > +        } else if (tries == 10) {
> > > > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > +                           _("Failed to access '%s' after %zu
> > > > tries"),
> > > > +                           path, tries);
> > > > +            return -1;
> > > > +        } else {
> > > > +            VIR_DEBUG("Failed to access '%s', re-try in %zu ms",
> > > > path, ms);
> > > > +            usleep(ms * 1000);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > >
> > > Just FYI, there's another way to address it by calling udevadm
> > > settle
> > > before and after "touching" a block device, libguestfs is using
> > > this
> > > approach and it works very well:
> > >
> > > https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=ud
> > > ev_s
> > > ettle&type=
> >
> > Does it? udevadm settle waits for all the events to be processed, not
> > just the one that we want. The wait time would be unpredictable IMO.
> >
> > Michal
>
> Well it's a kind of brute-force approach but at least guarantees the
> device will be accessible once it exits. Why would setting a custom
> arbitrary timeout be better which may be too short? Is the predictable
> wait time here more important than being able to open the device at
> all?
>

So, there was this thing about udevadm settle that wasn't clear to me, because
it says it waits for udevd to create all the device nodes - so since udev only
manages "/dev", I assumed it wouldn't work for sysfs attributes, but gave it a
try since I wasn't sure and, no, we can't use udevadm here, because udevd really
doesn't care about sysfs (mdevs only live in sysfs - well, strictly speaking,
a corresponding /dev/vfio/ device IS created for an mdev, but for that we get a
separate event and because for node device purposes we don't report vfio
devices, we ignore such event) hence the explicit, unpredictable, best-effort
timeout here.

Erik