| Message ID | 1445883177-11795-1-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 :)
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
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
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
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 ===
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(-)