[libvirt] [PATCHv2 2/5] conf: fix detection of infinite backing loop

[Peter Krempa]

On 04/08/14 14:39, Eric Blake wrote:
> On 04/08/2014 06:35 AM, Peter Krempa wrote:
>> On 04/08/14 06:07, Eric Blake wrote:
>>> While trying to refactor the backing file chain, I noticed that
>>> if you have a self-referential qcow2 file via a relative name:
>>>
>>> qemu-img create -f qcow2 loop 10M
>>> qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop
>>>
>>> then libvirt was creating a chain 2 deep before realizing it
>>> had hit a loop; furthermore, virStorageFileChainCheckBroken
>>> was not identifying the chain as broken.  With this patch,
>>> the loop is detected when the chain is only 1 deep; still
>>> enough for storage volume XML to display the file, but now
>>> with a proper error report about where the loop was found.
>>>
>>> * src/util/virstoragefile.c (virStorageFileGetMetadataRecurse):
>>> Add parameter, require canonical path up front.  Mark chain
>>> broken on OOM or loop detection.
>>> (virStorageFileGetMetadata): Pass in canonical name.
>>>
>>> Signed-off-by: Eric Blake <eblake at redhat.com>
>>> ---
>>>
>>> v2: hoist canonical computation out of virStorageFileGetMetadataRecurse
>>> rest of patch series still works as is
>>>
> 
>>>  /* Recursive workhorse for virStorageFileGetMetadata.  */
>>>  static virStorageFileMetadataPtr
>>> -virStorageFileGetMetadataRecurse(const char *path, const char *directory,
>>> +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,
>>
>> Hmm, I think that passing in canonical path isn't necessary here. The
>> path argument is from the second iteration canonicalized by the metadata
>> retrieval function. So to avoid the bug you IMO just need to
>> canonicalize the first call of this function. It's true that it would
>> actually change the path as reported by debug/error messages but I don't
>> think that should be problematic.
> 
> I actually want to pass BOTH names, through the ENTIRE call stack - the
> short name for error reporting, but the canonical name for open()
> operations.  The short name may be relative, but it is relative to the
> parent and NOT the current working directory, so it is unsuitable for
> open(); but the canonical name resolves symlinks and and is therefore
> unsuitable for easy correlation back to the names specified by the user
> or the qcow2 metadata.  I've got another patch coming that updates the
> entire call stack to use both names, not just the
> virStorageFileGetMetadataRecurse() function.
> 
> 
>>>      if (format <= VIR_STORAGE_FILE_NONE)
>>>          format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
>>> -    ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid,
>>> -                                           allow_probe, cycle);
>>> +    ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format,
>>
>> ... Here I'd change path for canonPath and would not refactor the rest.
>>
>>> +                                           uid, gid, allow_probe, cycle);
>>> + cleanup:
>>> +    VIR_FREE(canonPath);
>>>      virHashFree(cycle);
>>>      return ret;
>>>  }
>>
>>
>> Is there any reason you chose to add the parameter?
> 
> I could pass just canonPath here, to avoid the signature change in this
> patch, and save the entire signature change for the coming patch that
> passes both names down the entire call stack.
> 

Ah, okay then.

ACK.

Peter

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