Re: [Qemu-devel] [PATCH] block: allow best-effort query

[John Snow <jsnow@redhat.com>]

On 10/26/2015 05:18 PM, Max Reitz wrote:
> On 26.10.2015 19:12, John Snow wrote:
>> For more complex BDS trees that can be created under normal circumstances,
>> we lose the ability to issue query commands because of our inability to
>> re-construct the absolute filename.
>>
>> Instead, omit this field when it is a problem and present as much information
>> as we can.
>>
>> This will change the expected output in iotest 110, where we will now see a
>> json filename and the lack of an absolute filename instead of an error.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/qapi.c               | 10 ++++++----
>>  tests/qemu-iotests/110.out |  5 ++++-
>>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> One problem I see now is that qemu-img --backing-chain uses the full
> backing name if it is available and if it isn't, it resorts to the
> non-full backing name (which was good until this patch, as far as I can
> see). But now that's not necessarily a valid substitution anymore:
> 
> $ mkdir foo
> $ ./qemu-img create -f qcow2 foo/base.qcow2 64M
> $ ./qemu-img create -f qcow2 -b base.qcow2 foo/top.qcow2
> $ ./qemu-img create -f qcow2 base.qcow2 64M
> $ ./qemu-img info --backing-chain foo/top.qcow2
> image: foo/top.qcow2
> file format: qcow2
> virtual size: 64M (67108864 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: base.qcow2 (actual path: foo/base.qcow2)
> Format specific information:
>     compat: 1.1
>     lazy refcounts: false
>     refcount bits: 16
>     corrupt: false
> 
> image: foo/base.qcow2
> file format: qcow2
> virtual size: 64M (67108864 bytes)
> disk size: 196K
> cluster_size: 65536
> Format specific information:
>     compat: 1.1
>     lazy refcounts: false
>     refcount bits: 16
>     corrupt: false
> $ ./qemu-img info --backing-chain \
> "json:{'file.filename':'foo/top.qcow2',
>        'driver':'qcow2','lazy-refcounts':true}"
> image: json:{"lazy-refcounts": true, "driver": "qcow2", "file":
> {"driver": "file", "filename": "foo/top.qcow2"}}
> file format: qcow2
> virtual size: 64M (67108864 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: base.qcow2
> Format specific information:
>     compat: 1.1
>     lazy refcounts: false
>     refcount bits: 16
>     corrupt: false
> 
> image: base.qcow2
> file format: qcow2
> virtual size: 32M (33554432 bytes)
> disk size: 196K
> cluster_size: 65536
> Format specific information:
>     compat: 1.1
>     lazy refcounts: false
>     refcount bits: 16
>     corrupt: false
> 
> As you can see, in the second case, the wrong base.qcow2 was used. I
> think qemu-img info --backing-chain should abort once it hits a point
> where has_full_backing_filename is false; but for that to work, we need
> to set info->full_backing_filename even if the "relative" and the
> absolute backing filename (backing_filename and backing_filename2) are
> equal.
> 
> (By the way: It's actually some kind of a bug that you can open images
> with JSON filenames and backing files; this only works because
> bdrv_open_backing_file() is called before bdrv_refresh_filename().
> Actually, after my "Drop BDS.filename" series it will no longer work.
> This is bad. I need to fix this before I can continue the series...)
> 
> ((It is bad because that means:
> $ x86_64-softmmu/qemu-system-x86_64 \
>   -drive if=none,file.filename=foo/top.qcow2,lazy-refcounts=on
> qemu-system-x86_64: -drive
> if=none,file.filename=foo/top.qcow2,lazy-refcounts=on: Cannot use
> relative backing file names for 'json:{"lazy-refcounts": "on", "driver":
> "qcow2", "file": {"driver": "file", "filename": "foo/top.qcow2"}}'
> 
> And that's bad.))
> 
> (((So for this patch that means I guess we should just fix
> bdrv_get_full_backing_filename() instead.)))
> 
> Max
> 

Sigh, I guess there's no easy fixes within ten miles of this fifty car
pileup.

What's your opinion for what a meaningful fix to
bdrv_get_full_backing_filename might be?

It's not really rational for query to ever _fail_; I would be tempted to
fix any users of query information to understand what to do if that
field is absent.

--js