qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] snapshot: fixed bdrv_get_full_backing_filename


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH] snapshot: fixed bdrv_get_full_backing_filename can not get correct full_backing_filename
Date: Mon, 10 Nov 2014 13:37:01 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]