[libvirt] [PATCHv2 4/5] storage: don't follow backing chain symlinks too eagerly

[Peter Krempa]

On 02/15/13 21:38, Eric Blake wrote:
> If you have a qcow2 file /path1/to/file pointed to by symlink
> /path2/symlink, and pass qemu /path2/symlink, then qemu treats
> a relative backing file in the qcow2 metadata as being relative
> to /path2, not /path1/to.  Yes, this means that it is possible
> to create a qcow2 file where the choice of WHICH directory and
> symlink you access its contents from will then determine WHICH
> backing file (if any) you actually find; the results can be
> rather screwy, but we have to match what qemu does.
>
> Libvirt and qemu default to creating absolute backing file
> names, so most users don't hit this.  But at least VDSM uses
> symlinks and relative backing names alongside the
> --reuse-external flags to libvirt snapshot operations, with the
> result that libvirt was failing to follow the intended chain of
> backing files, and then backing files were not granted the
> necessary sVirt permissions to be opened by qemu.
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for
> more gory details.  This fixes a regression introduced in
> commit 8250783.
>
> I tested this patch by creating the following chain:
>
> ls /home/eblake/Downloads/Fedora.iso # raw file for base
> cd /var/lib/libvirt/images
> qemu-img create -f qcow2 \
>    -obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one
> mkdir sub
> cd sub
> ln -s ../one onelink
> qemu-img create -f qcow2 \
>    -obacking_file=../sub/onelink,backing_fmt=qcow2 two
> mv two ..
> ln -s ../two twolink
> qemu-img create -f qcow2 \
>    -obacking_file=../sub/twolink,backing_fmt=qcow2 three
> mv three ..
> ln -s ../three threelink
>
> then pointing my domain at /var/lib/libvirt/images/sub/threelink.
> Prior to this patch, I got complaints about missing backing
> files; afterwards, I was able to verify that the backing chain
> (and hence DAC and SELinux relabels) of the entire chain worked.
>
> * src/util/virstoragefile.h (_virStorageFileMetadata): Add
> directory member.
> * src/util/virstoragefile.c (absolutePathFromBaseFile): Drop,
> replaced by...
> (virFindBackingFile): ...better function.
> (virStorageFileGetMetadataInternal): Add an argument.
> (virStorageFileGetMetadataFromFD, virStorageFileChainLookup)
> (virStorageFileGetMetadata): Update callers.
> ---
>   src/util/virstoragefile.c | 88 ++++++++++++++++++++++++++++++-----------------
>   src/util/virstoragefile.h |  1 +
>   2 files changed, 57 insertions(+), 32 deletions(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index d741d3d..5bb2b58 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -497,44 +497,56 @@ qedGetBackingStore(char **res,
>   }
>
>   /**
> - * Return an absolute path corresponding to PATH, which is absolute or relative
> - * to the directory containing BASE_FILE, or NULL on error
> + * Given a starting point START (either an original file name, or the
> + * directory containing the original name, depending on START_IS_DIR)
> + * and a possibly relative backing file NAME, compute the relative
> + * DIRECTORY (optional) and CANONICAL (mandatory) location of the
> + * backing file.  Return 0 on success, negative on error.
>    */
> -static char *
> -absolutePathFromBaseFile(const char *base_file, const char *path)
> +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5)
> +virFindBackingFile(const char *start, bool start_is_dir, const char *path,
> +                   char **directory, char **canonical)
>   {
> -    char *res = NULL;
> -    char *tmp = NULL;
> -    size_t d_len = dir_len(base_file);
> +    char *combined = NULL;
> +    int ret = -1;
>
> -    /* If path is already absolute, or if dirname(base_file) is ".",
> -       just return a copy of path.  */
> -    if (*path == '/' || d_len == 0) {
> -        if (!(res = canonicalize_file_name(path)))
> -            virReportSystemError(errno,
> -                                 _("Can't canonicalize path '%s'"), path);
> +    if (*path == '/') {
> +        /* Safe to cast away const */
> +        combined = (char *)path;
> +    } else {
> +        size_t d_len = start_is_dir ? strlen(start) : dir_len(start);
>
> -        goto cleanup;
> +        if (d_len > INT_MAX) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("name too long: '%s'"), start);
> +            goto cleanup;
> +        } else if (!d_len) {

I'd go for (d_len == 0) here so that it will not be confused with pointers.

> +            start = ".";
> +            d_len = 1;
> +        }
> +        if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
>       }

Otherwise looks reasonable.

ACK.

Peter