Chris Lalancette <clalance@xxxxxxxxxx> 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" ;-)
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list