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

[Max Reitz <mreitz@redhat.com>]

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