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

[Erik Skultety]

On Tue, Jun 20, 2017 at 05:22:52PM +0200, Peter Krempa wrote:
> On Tue, Jun 20, 2017 at 17:03:31 +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.
>
> This statement is useless. The helper function can be reused elsewhere.
>
> >
> > 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/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).
>
> This description does not make sense. You don't state that this reports
> errors. Also the mention of ENOENT does not make sense.
>
> This function in fact sometimes returns enoent if the file does not
> appear until timeout.
>
> > + */
> > +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);
>
> This does not really explain stuff to users. You might want to add a
> more comprehensive error message or leave error reporting to users.
>
> > +            return -1;
> > +        } else if (tries == 10) {
>
> This does not make any sense. The while loop counts down and checks if
> tries is more than 10. And this checks that tries is equal to 10.
>
> So if somebody passes 11 as @tries he/she gets this error? If tries is
> set to < 10 you get success even on timeout?
>
> Did you modify the code without testing it?

Damn :/, I actually tested it (after those 3+ modifications I did to it in an
iterative manner) and it worked, but surely didn't hit the issue you describe.
Nevertheless, yep, it's wrong and I need to rework it.