From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmlYr-0002LX-JP for qemu-devel@nongnu.org; Fri, 07 Nov 2014 10:34:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XmlYm-0001DY-BJ for qemu-devel@nongnu.org; Fri, 07 Nov 2014 10:34:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45294) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmlYm-0001DA-47 for qemu-devel@nongnu.org; Fri, 07 Nov 2014 10:34:48 -0500 Message-ID: <545CE692.7020603@redhat.com> Date: Fri, 07 Nov 2014 16:34:42 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415371697-27908-1-git-send-email-junmuzi@gmail.com> In-Reply-To: <1415371697-27908-1-git-send-email-junmuzi@gmail.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jun Li , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, juli@redhat.com, stefanha@redhat.com 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 > --- > 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);