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

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

[Jun Li]

This patch fixed the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .
path_combine can not calculate the correct full path name for backing_file.
Such as:
create a snapshot chain:
sn2->sn1->$BASE_IMG
backing file is : /home/wookpecker/img.qcow2
sn1 : /home/woodpecker/tmp/sn1
sn2 : /home/woodpecker/tmp/sn2
when create sn2, path_combine can not got a correct path for $BASE_IMG.

Signed-off-by: Jun Li <junmuzi@gmail.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 990a754..2519f68 100644
--- a/block.c
+++ b/block.c
@@ -307,7 +307,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);
+        realpath(bs->backing_file, dest);
     }
 }
 
-- 
1.8.3.1

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

[Jun Li]

This patch fixed the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .
path_combine can not calculate the correct full path name for backing_file.
Such as:
create a snapshot chain:
sn2->sn1->$BASE_IMG
backing file is : /home/wookpecker/img.qcow2
sn1 : /home/woodpecker/tmp/sn1
sn2 : /home/woodpecker/tmp/sn2
when create sn2, path_combine can not got a correct path for $BASE_IMG.

Signed-off-by: Jun Li <junmuzi@gmail.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 990a754..2519f68 100644
--- a/block.c
+++ b/block.c
@@ -307,7 +307,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);
+        realpath(bs->backing_file, dest);
     }
 }
 
-- 
1.8.3.1

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On 04/05/2014 10:43 AM, Jun Li wrote:
> This patch fixed the following bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1084302 .
> path_combine can not calculate the correct full path name for backing_file.
> Such as:
> create a snapshot chain:
> sn2->sn1->$BASE_IMG
> backing file is : /home/wookpecker/img.qcow2
> sn1 : /home/woodpecker/tmp/sn1
> sn2 : /home/woodpecker/tmp/sn2
> when create sn2, path_combine can not got a correct path for $BASE_IMG.
> 
> Signed-off-by: Jun Li <junmuzi@gmail.com>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 990a754..2519f68 100644
> --- a/block.c
> +++ b/block.c
> @@ -307,7 +307,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);
> +        realpath(bs->backing_file, dest);

Ouch - I think this does the wrong thing in the presence of symlinks.

VDSM is an existing client that likes to create symlinks to qcow2 files,
where the backing chain uses relative names that work ONLY if you
resolve them in relation to the directory containing the symlink.  As an
example, if I have the following tree:

/path/to/base         - qcow2 file
/path/to/mid          - qcow2 file, backing of '../dir/baselink'
/path/to/top          - qcow2 file, backing of '../dir/midlink'
/path/to/dir/baselink - symlink to '../base'
/path/to/dir/midlink  - symlink to '../mid'
/path/to/dir/toplink  - symlink to '../top'

then changing directories to '/path/to/dir' and opening './toplink'
should correctly find ../dir/midlink and ../dir/../dir/baselink (aka
../mid and ../top) as its backing chain.  But if you use realpath() on
the results, you will get '/path/to/mid' as the backing file of
'toplink' (that is, the symlink is canonicalized), and when you then go
to find the backing file, '/path/to/../dir/baselink' does NOT exist (by
using realpath, you are starting the search for the relative backing
file from the wrong directory).


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Jun Li]

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.

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 <junmuzi@gmail.com>
---
 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);
-- 
1.9.3

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 <junmuzi@gmail.com>
> ---
>   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);

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

[Jun Li]

On Fri, 11/07 16:34, Max Reitz wrote:
> 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
> 

Hi Max,

  Do above two patches can resolve this bz https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2 ? If it can solve this bz, so please ignore my patch.

If not, why not using absolute filename to instead relative filename to the
"-b" option during qemu-img create? Using absolute filename just need more
spaces for backing_file of structure BlockDriverState.

BTW, the above two patches are very necessary, I think.

Regards,
Jun Li

> 
> >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 <junmuzi@gmail.com>
> >---
> >  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);
> 

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

[Max Reitz]

On 2014-11-10 at 09:19, Jun Li wrote:
> On Fri, 11/07 16:34, Max Reitz wrote:
>> 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
>>
> Hi Max,
>
>    Do above two patches can resolve this bz https://bugzilla.redhat.com/show_bug.cgi?id=1161582#c2 ?

No, I don't think so.

However, the real reason for the problem you're describing in that 
comment 2 is probably that the blockdev-snapshot-sync implementation 
directly uses the filename of the BDS to be synced instead of first 
making it an absolute filename or at least ensuring that it is an 
absolute filename. To do this, the bdrv_img_create() call in 
external_snapshot_prepare() in blockdev.c probably should not give 
state->old_bs->filename directly as the backing filename, but make it 
absolute first (by using realpath(), I guess).

We should not bother to try to find an equivalent relative filename to 
the target file.

> If it can solve this bz, so please ignore my patch.
>
> If not, why not using absolute filename to instead relative filename to the
> "-b" option during qemu-img create? Using absolute filename just need more
> spaces for backing_file of structure BlockDriverState.

And it doesn't work anymore if I want to have a relative filename.

Suppose I want to have, for whatever reason, some image, and two other 
images which use that image as a backing file. Further suppose that I 
want to be able to distribute these three images in an archive. Without 
using a relative filename for the backing file, that won't work.

I personally want to be able to use relative backing filenames and 
easiest way to do that is by directly storing the string given to 
qemu-img create for the "-b" option in the backing filename field of the 
file to be created. This is not very intuitive, as I said before, 
because that path is therefore relative to the directory of the new file 
and not relative to qemu-img's working directory; but changing this 
behavior would break compatibility and in my opinion no real gain (other 
than being able to use tab completion, but I remember there are some git 
commands which always require a pass relative to the repository's root 
directory, independently of where your current working directory is; so 
qemu-img is at least not the only program which does not always 
interpret a relative filename relatively to its current working directory).

A solution to this problem may be to always try to open the backing file 
in bdrv_img_create (not only when the size is omitted) so that the user 
immediately sees that the path given is wrong. However, I can imagine 
some users actually wanting to reference a non-existing backing file and 
creating that file later on, so we probably don't want that either.

I'd just leave image creation as it is, and for the above bug report 
(for the problem described in your comment 2) fix 
external_snapshot_prepare() in blockdev.c

Max