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

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

[John Snow]

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

diff --git a/block/qapi.c b/block/qapi.c
index 355ba32..c4101d5 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -245,13 +245,15 @@ void bdrv_query_image_info(BlockDriverState *bs,
         info->has_backing_filename = true;
         bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
         if (err) {
-            error_propagate(errp, err);
-            qapi_free_ImageInfo(info);
+            /* Couldn't reconstruct that backing filename,
+             * So we'll skip this field, but apply a Best Effort to the query */
             g_free(backing_filename2);
-            return;
+            backing_filename2 = NULL;
+            error_free(err);
         }
 
-        if (strcmp(backing_filename, backing_filename2) != 0) {
+        if (backing_filename2 &&
+            strcmp(backing_filename, backing_filename2) != 0) {
             info->full_backing_filename =
                         g_strdup(backing_filename2);
             info->has_full_backing_filename = true;
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 0270980..425485f 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -11,7 +11,10 @@ backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
 
 === Non-reconstructable filename ===
 
-qemu-img: Cannot use relative backing file names for 'json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "set-state.0.new_state": 42}}'
+image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "set-state.0.new_state": 42}}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base
 
 === Backing name is always relative to the backed image ===
 
-- 
2.4.3

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On 10/26/2015 12:12 PM, 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Partial is indeed better than nothing :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3645 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[John Snow]

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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5476 bytes --]

On 26.10.2015 22:34, John Snow wrote:
> 
> 
> 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.

After having thought about it, my current plan is this: Every protocol
driver can provide a function which basically returns a base directory
for a given BDS. If no such function is provided, we use something like
dirname(BDS.options['filename']) + '/'. A format BDS's base directory is
its file's base directory.

Then, if we have a relative backing filename, we use that function to
query the base directory. If we don't receive one (there may be block
drivers where it's simply not applicable, like quorum), then we still
fail. But we'd fail much less often, and only if there's really no way
around it.

So I think this patch is in principle still fine because
bdrv_get_full_backing_filename() can still fail (if your top BDS is
handled by a block driver which simply cannot use normal filenames, but
you are trying to attach a backing file with a relative filename).
Especially since we won't get the other stuff in before 2.5.

But I'd still like the qemu-img info --backing-chain stuff fixed. It's
not critical, but it shouldn't be too hard to fix it either.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]