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

[Ján Tomko]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140320/1e5cf610/attachment-0001.sig>