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

[Peter Krempa]

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?


> +            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);

This will spam logs too much. I think a debug prior to the while loop
and one after it succeeds should be enough.

> +            usleep(ms * 1000);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170620/975725df/attachment-0001.sig>