From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqpPA-00025o-Cj for qemu-devel@nongnu.org; Mon, 26 Oct 2015 17:34:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqpP9-0004JJ-7V for qemu-devel@nongnu.org; Mon, 26 Oct 2015 17:34:12 -0400 References: <1445883177-11795-1-git-send-email-jsnow@redhat.com> <562E989F.5010000@redhat.com> From: John Snow Message-ID: <562E9C4A.2010304@redhat.com> Date: Mon, 26 Oct 2015 17:34:02 -0400 MIME-Version: 1.0 In-Reply-To: <562E989F.5010000@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: allow best-effort query List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org 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 >> --- >> 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