On 03/20/2014 10:50 AM, Ján Tomko wrote: > 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. Okay, it looks like you've done your due diligence on all the paths, so I'm satisfied. ACK with the addition to the commit message. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list