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(a)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