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

[Laine Stump]

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.