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 <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);
>