[libvirt] [PATCH]: Give /dev/disk/by-{id,path} a chance to exist

[Jim Meyering]

Chris Lalancette <clalance at redhat.com> wrote:
...
> on-demand by udev.  It's basically a race between udev creating the directory
> and libvirt scanning the directory.
>      The following patch just adds a retry to the code above so that if it
> doesn't exist when we first get there, we give it time to come into being.  If
> it still hasn't shown up 5 seconds after we started, we give up and throw the error.
...
> -    if ((dh = opendir(pool->def->target.path)) == NULL) {
> +    dh = NULL;

not needed?

> +    opentries = 0;
> +    while (opentries < 50) {
> +        if ((dh = opendir(pool->def->target.path)) != NULL)
> +            break;

Hi Chris,

Nice one.  Must have been er,.. fun to track down.
The patch looks fine, but how about making it so that it retries
only for a missing final (ENOENT) or intermediate (ENOTDIR)
component.  We'd rather not have it retry due to OOM or lack of
permission, e.g.,

    dh = opendir(pool->def->target.path);
    if (dh != NULL || (errno != ENOENT && errno != ENOTDIR))
        break;

but that has so many 'not's that you might prefer to write it this way:

        if (! (dh == NULL && (errno == ENOENT || errno == ENOTDIR)))
            break;

You can save a two lines by using a for loop.
It's nice to put a name on the constant, e.g.,

    enum { WAIT_FOR_UDEV_SECS = 5 };

(and enum is better than #define, because it's usable in a debugger)

    for (open_tries = 0; open_tries < WAIT_FOR_UDEV_SECS * 10; open_tries++) {
        dh = opendir(pool->def->target.path);
        /* Stop if opendir succeeds, or if it fails for any
           reason other than a missing file name component.  */
        if (dh != NULL || (errno != ENOENT && errno != ENOTDIR))
            break;
        usleep(100 * 1000);
    }

Why "open_tries"?
Because I first read "opentries" as "op" "entries" ;-)