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

[Peter Krempa]

On 04/06/14 05:32, 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):
> Mark chain broken if OOM or infinite loop occurs.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/util/virstoragefile.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 017717c..39e4c19 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1074,15 +1074,21 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory,

You are adding this code into the recursive worker. It's possible only
in the first iteration here to get a non-canonical "path" here, as all
other backing chain elements are prepped with virFindBackingFile.

>                path, format, (int)uid, (int)gid, allow_probe);
> 
>      virStorageFileMetadataPtr ret = NULL;
> +    char *canonical = canonicalize_file_name(path);

Thus this call is redundant on all other iterations than the first.

> 
> -    if (virHashLookup(cycle, path)) {
> +    /* Hash by canonical name */
> +    if (!canonical) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +    if (virHashLookup(cycle, canonical)) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("backing store for %s is self-referential"),
>                         path);
> -        return NULL;
> +        goto cleanup;
>      }
> -    if (virHashAddEntry(cycle, path, (void *)1) < 0)
> -        return NULL;
> +    if (virHashAddEntry(cycle, canonical, (void *)1) < 0)
> +        goto cleanup;
> 
>      if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
>          virReportSystemError(-fd, _("Failed to open file '%s'"), path);

Please move the canonicalization code into virStorageFileGetMetadata so
that it doesn't get called multiple times. (And also one of my planed
refactors need to change this function to track remote images too and
this would definitely be changed afterwards).

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/20140407/5334a193/attachment-0001.sig>