[libvirt] [PATCHv4] Ignore missing files on pool refresh

[Laine Stump]

On 03/20/2014 09:57 AM, Ján Tomko wrote:
> If we cannot stat/open a file on pool refresh, returning -1 aborts
> the refresh and the pool is undefined.
>
> Don't treat missing files as fatal unless VolOpenCheckMode is called
> with the VIR_STORAGE_VOL_OPEN_ERROR flag.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=977706
> ---
> v1: https://www.redhat.com/archives/libvir-list/2013-July/msg00635.html
> v2: https://www.redhat.com/archives/libvir-list/2013-July/msg00639.html
> v3: https://www.redhat.com/archives/libvir-list/2013-July/msg01026.html
> (by Guanan Ren, also checked the 'open' call)
> v4: do not call open on sockets and fifos and only skip missing files
>     if we're doing a pool refresh, otherwise 'vol-info' on a missing
>     volume returns 'unknown error'
>
>  src/storage/storage_backend.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index d14e633..97ab7b8 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1212,6 +1212,10 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
>      char *base = last_component(path);
>  
>      if (lstat(path, sb) < 0) {
> +        if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
> +            VIR_WARN("ignoring missing file '%s'", path);
> +            return -2;
> +        }

This return code bubbles up to *a lot* of places; I've been tracking
through them (with cscope) for a few minutes and haven't yet found the
one that treats -2 different from -1 (I'm sure it's there, I just
haven't gotten there yet). In the meantime, it seems like there are many
places where a return of -2 is considered a failure, but we won't have
logged the error in these cases.

What can you say to convince me that this isn't actually true? (It's
possible that I'm missing some key point, because I haven't spent as
much time staring at this code :-)

(Also, even if I am missing something on the point of "missing log
messages when this really is an error", it would be good for the commit
log to point out the places where -2 is interpreted differently.)


>          virReportSystemError(errno,
>                               _("cannot stat file '%s'"),
>                               path);
> @@ -1238,6 +1242,10 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
>              VIR_WARN("ignoring dangling symlink '%s'", path);
>              return -2;
>          }
> +        if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
> +            VIR_WARN("ignoring missing file '%s'", path);
> +            return -2;
> +        }
>  
>          virReportSystemError(errno,
>                               _("cannot open volume '%s'"),