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@xxxxxxxxxx> >> --- >> >> 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list