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@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