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

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(a)redhat.com&gt; > --- > > 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.

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