Re: [Qemu-devel] [PATCH] snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename

[Max Reitz]

On 2014-11-07 at 15:48, Jun Li wrote:
> When bs->filename and bs->backing_file are relative pathname and not under the
> same directory, path_combine() can not give the correct path for
> bs->backing_file. So add get_localfile_absolute_path to get absolute path for
> local file.

Well, for me it is the correct path. I'm using

$ mkdir -p foo
$ qemu-img create -f qcow2 foo/backing.qcow2 64M
$ qemu-img create -f qcow2 -b backing.qcow2 foo/backed.qcow2

I guess this patch will break that?

I know this isn't ideal, but that's just how it works, so we would break 
existing usage.

Furthermore, compiling with this patch fails for me because on my system 
readlink() and realpath() have the attribute warn_unused_result. Also, 
I'm not sure, but readlink() may only resolve one link (which may point 
to another link in turn).

And finally, with this patch applied and the return value issue fixed:

$ mkdir -p foo
$ qemu-img create -f qcow2 foo/backing.qcow2 64M
$ qemu-img create -f qcow2 -b foo/backing.qcow2 foo/backed.qcow2 64M
$ cd foo
$ qemu-img info backed.qcow2
lstat: No such file or directory

Because the backing field in the qcow2 file points to 
"foo/backing.qcow2" which does not exist.

If you are giving a relative filename to the "-b" option during qemu-img 
create, this filename is not (contrary to intuition) relative to the 
current working directory of qemu-img, but it is the value which is put 
directly into the backing field of the file to be created; and that 
value is relative to that file's directory.

See also 
http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00632.html 
and http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00633.html

Max

> e.g:
> $ pwd
> /tmp
> $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./test 5M
> Formatting './test', fmt=qcow2 size=5242880 encryption=off cluster_size=65536
> lazy_refcounts=off
> $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test1 -b ./test
> Formatting './tmp/test1', fmt=qcow2 size=5242880 backing_file='./test'
> encryption=off cluster_size=65536 lazy_refcounts=off
> $ /opt/qemu-git-arm/bin/qemu-img create -f qcow2 ./tmp/test2 -b ./tmp/test1
> qemu-img: ./tmp/test2: Could not open './tmp/test1': Could not open backing
> file: Could not open './tmp/./test': No such file or directory: No such file
> or directory
>
> This patch also fixes the following bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2
>
> Signed-off-by: Jun Li <address@hidden>
> ---
>   block.c               | 28 +++++++++++++++++++++++++++-
>   include/block/block.h |  2 ++
>   2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index dacd881..5e2f669 100644
> --- a/block.c
> +++ b/block.c
> @@ -259,6 +259,32 @@ int path_is_absolute(const char *path)
>   #endif
>   }
>   
> +void get_localfile_absolute_path(char *dest, int dest_size,
> +                                 const char *filename)
> +{
> +    struct stat sb;
> +    char *linkname;
> +
> +    if (path_is_absolute(filename)) {
> +        pstrcpy(dest, dest_size, filename);
> +    } else {
> +        if (lstat(filename, &sb) == -1) {
> +            perror("lstat");
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        /* Check linkname is a link or not */
> +        if (S_ISLNK(sb.st_mode)) {
> +            linkname = malloc(sb.st_size + 1);
> +            readlink(filename, linkname, sb.st_size + 1);
> +            linkname[sb.st_size] = '\0';
> +            realpath(linkname, dest);
> +        } else {
> +            realpath(filename, dest);
> +        }
> +    }
> +}
> +
>   /* if filename is absolute, just copy it to dest. Otherwise, build a
>      path to it by considering it is relative to base_path. URL are
>      supported. */
> @@ -308,7 +334,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, 
> char *dest, size_t sz)
>       if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file)) {
>           pstrcpy(dest, sz, bs->backing_file);
>       } else {
> -        path_combine(dest, sz, bs->filename, bs->backing_file);
> +        get_localfile_absolute_path(dest, sz, bs->backing_file);
>       }
>   }
>   
> diff --git a/include/block/block.h b/include/block/block.h
> index 13e4537..6ddb150 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -398,6 +398,8 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
>   int bdrv_is_snapshot(BlockDriverState *bs);
>   
>   int path_is_absolute(const char *path);
> +void get_localfile_absolute_path(char *dest, int dest_size,
> +                                 const char *filename);
>   void path_combine(char *dest, int dest_size,
>                     const char *base_path,
>                     const char *filename);