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. However if you remove a file and replace it with a pipe, the 'unknown error' could happen, but that's pre-existing.
(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.)
Good point, I'll amend the commit message. Thanks for the review, Jan