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

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'"),

On 03/20/2014 05:19 PM, Laine Stump wrote: > 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 :-) My cscope shows five callers: [1] virStorageBackendVolOpen [2] virStorageBackendUpdateVolTargetInfo [3] virStorageBackendProbeTarget [4] virStorageBackendMpathUpdateVolTargetInfo [5] virStorageBackendSCSIUpdateVolTargetInfo [1][4][5] use VIR_STORAGE_VOL_OPEN_DEFAULT flags, which contain VIR_STORAGE_VOL_OPEN_ERROR. In this case -1 is returned and the error is reported. [3] is the reason for this patch and handles -2 correctly [2] is called twice in virStorageBackendUpdateVolInfoFlags: once with DEFAULT flags, and once with flags passed from the callers: [1] virStorageBackendUpdateVolInfo, using DEFAULT flags [2] in virStorageBackendFileSystemVolRefresh, using VIR_STORAGE_VOL_FS_OPEN_FLAGS So this patch should not add a case when we return -2 without the caller being ready to ignore the failure.