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@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. > Ah, okay then. ACK. Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list