On 03/20/2014 10:50 AM, Ján Tomko wrote:
On 03/20/2014 05:19 PM, Laine Stump 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
On 03/20/2014 09:57 AM, Ján Tomko wrote: 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.