From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqqAB-0000VK-D2 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 18:22:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqqAA-0007zR-6r for qemu-devel@nongnu.org; Mon, 26 Oct 2015 18:22:47 -0400 References: <1445883177-11795-1-git-send-email-jsnow@redhat.com> <562E989F.5010000@redhat.com> <562E9C4A.2010304@redhat.com> From: Max Reitz Message-ID: <562EA7AC.2040507@redhat.com> Date: Mon, 26 Oct 2015 23:22:36 +0100 MIME-Version: 1.0 In-Reply-To: <562E9C4A.2010304@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KiGTn7iEktgp3VVgviuawaQ0fUhDjdueP" Subject: Re: [Qemu-devel] [PATCH] block: allow best-effort query List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --KiGTn7iEktgp3VVgviuawaQ0fUhDjdueP Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 26.10.2015 22:34, John Snow wrote: >=20 >=20 > 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 circumsta= nces, >>> 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 inf= ormation >>> 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 erro= r. >>> >>> 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 ca= n >> 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 nee= d >> 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=3Dnone,file.filename=3Dfoo/top.qcow2,lazy-refcounts=3Don >> qemu-system-x86_64: -drive >> if=3Dnone,file.filename=3Dfoo/top.qcow2,lazy-refcounts=3Don: Cannot us= e >> 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 >> >=20 > Sigh, I guess there's no easy fixes within ten miles of this fifty car > pileup. >=20 > What's your opinion for what a meaningful fix to > bdrv_get_full_backing_filename might be? >=20 > It's not really rational for query to ever _fail_; I would be tempted t= o > 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 --KiGTn7iEktgp3VVgviuawaQ0fUhDjdueP Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWLqesAAoJEDuxQgLoOKytJRkH/Rh7qdwxvUlycP/KZc9dv8tr D+4aZpxt4aBtU81o1aYMkfSbJwd2lsHfsK2bIdZWcuD+fCvPKgf7kMp2dXr9OIOy ZbfM0FkAX6+kUrFq+FCRLG6A7vmaoOuwHiCkRvB3OcmTO2oBUpQdcA+Dh9UeG1t7 zqXYQ2PG2YiiRqhK+F1MmWZ3N9tfHuuvwxa3UvT9yl2xNYqQfGAoBG3JOomdQLUB FbpXNK6SBLU3H+yZzou+m0vB9JaqWiOZYA+HUeYhKAerZtkeqFt0gXEZnJk1C9al oZu0Cy3Mmkcab9fuJR5HdJbz1h2MluPS0rGlqHiyHrnMs5gS7Mkojr52xXi3i2M= =2puZ -----END PGP SIGNATURE----- --KiGTn7iEktgp3VVgviuawaQ0fUhDjdueP--