From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3R5Y-00072k-6k for qemu-devel@nongnu.org; Mon, 30 Nov 2015 11:14:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3R5T-00048r-V2 for qemu-devel@nongnu.org; Mon, 30 Nov 2015 11:14:04 -0500 References: <1448294400-476-1-git-send-email-kwolf@redhat.com> <1448294400-476-7-git-send-email-kwolf@redhat.com> <565899D4.1090907@redhat.com> <20151130090107.GA4494@noname.str.redhat.com> From: Max Reitz Message-ID: <565C75B9.7090302@redhat.com> Date: Mon, 30 Nov 2015 17:13:45 +0100 MIME-Version: 1.0 In-Reply-To: <20151130090107.GA4494@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="s7tLABHKtKVjtJbXOX6VdTvKrhFXq6A1o" Subject: Re: [Qemu-devel] [PATCH v2 06/21] block: Exclude nested options only for children in append_open_options() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --s7tLABHKtKVjtJbXOX6VdTvKrhFXq6A1o Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 30.11.2015 10:01, Kevin Wolf wrote: > Am 27.11.2015 um 18:58 hat Max Reitz geschrieben: >> On 23.11.2015 16:59, Kevin Wolf wrote: >>> Some drivers have nested options (e.g. blkdebug rule arrays), which >>> don't belong to a child node and shouldn't be removed. Don't remove a= ll >>> options with "." in their name, but check for the complete prefixes o= f >>> actually existing child nodes. >>> >>> Signed-off-by: Kevin Wolf >>> --- >>> block.c | 19 +++++++++++++++---- >>> include/block/block_int.h | 1 + >>> 2 files changed, 16 insertions(+), 4 deletions(-) >> >> Thanks, now I don't need to fix it myself. :-) >> >> (I would have had to do that for an in-work series of mine) >> >>> diff --git a/block.c b/block.c >>> index 23d9e10..02125e2 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options,= const char **pfilename, >>> =20 >>> static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, >>> BlockDriverState *child_bs, >>> + const char *child_name, >>> const BdrvChildRole *child_role)= >>> { >>> BdrvChild *child =3D g_new(BdrvChild, 1); >>> *child =3D (BdrvChild) { >>> .bs =3D child_bs, >>> + .name =3D child_name, >>> .role =3D child_role, >>> }; >>> =20 >>> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, = BlockDriverState *backing_hd) >>> bs->backing =3D NULL; >>> goto out; >>> } >>> - bs->backing =3D bdrv_attach_child(bs, backing_hd, &child_backing= ); >>> + bs->backing =3D bdrv_attach_child(bs, backing_hd, "backing", &ch= ild_backing); >>> bs->open_flags &=3D ~BDRV_O_NO_BACKING; >>> pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->= filename); >>> pstrcpy(bs->backing_format, sizeof(bs->backing_format), >>> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename= , >>> goto done; >>> } >>> =20 >>> - c =3D bdrv_attach_child(parent, bs, child_role); >>> + c =3D bdrv_attach_child(parent, bs, bdref_key, child_role); >>> =20 >>> done: >>> qdict_del(options, bdref_key); >>> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, Blo= ckDriverState *bs) >>> { >>> const QDictEntry *entry; >>> QemuOptDesc *desc; >>> + BdrvChild *child; >>> bool found_any =3D false; >>> + const char *p; >>> =20 >>> for (entry =3D qdict_first(bs->options); entry; >>> entry =3D qdict_next(bs->options, entry)) >>> { >>> - /* Only take options for this level */ >>> - if (strchr(qdict_entry_key(entry), '.')) { >>> + /* Exclude options for children */ >>> + QLIST_FOREACH(child, &bs->children, next) { >>> + if (strstart(qdict_entry_key(entry), child->name, &p) >>> + && (!*p || *p =3D=3D '.')) >>> + { >>> + break; >>> + } >>> + } >>> + if (child) { >>> continue; >>> } >>> =20 >> >> A good general solution, but I think bdrv_refresh_filename() may be ba= d >> enough to break with general solutions. ;-) >> >> bdrv_refresh_filename() only considers "file" and "backing" (actually,= >> it only supports "file" for now, I'm working on "backing", though). Th= e >> only drivers with other children are quorum, blkdebug, blkverify and >> VMDK. The former three have their own implementation of >> bdrv_refresh_filename(), so they don't use append_open_options() at al= l. >> The latter, however, (VMDK) does not. >> >> This change to append_open_options results in the extent.%d options >> simply being omitted altogether because bdrv_refresh_filename() does n= ot >> fetch them. Before, they were included in the VMDK BDS's options, whic= h >> is not ideal but works more or less. >=20 > Are you sure? As far as I can tell, this patch should only keep options= > that were previously removed, but not remove options that were > previously kept (with the exception of direct use of child names, which= > I added here to address your review comments for v1). >=20 > Specifically for "extents.%d", this is a child name and is therefore > omitted. However, it contains a '.', so it was already removed without > this patch. Right, it is broken already. The same applies to qcow2's options containing a dot. Max > I'm accepting proof of the contrary in the form of a test case. ;-) >=20 >> In order to "fix" this, I see three ways right now: >> 1. Just care about "file" and "backing". bdrv_refresh_filename() >> doesn't support anything else, so that will be fine. >> 2. Implement bdrv_refresh_filename() specifically for VMDK so >> append_open_options() will never have to handle anything but "file"= >> and "backing". >> 3. Fix bdrv_refresh_filename() so that it handles all children and not= >> just "file" and "backing". >> >> Since we are shooting for 2.6 anyway (I assume ;-)), I think we should= >> go for option 3. This means that this patch is fine, and I'll see to >> fixing bdrv_refresh_filename() (because I'm working on that anyway). >=20 > Yes, I agree. >=20 > Kevin >=20 --s7tLABHKtKVjtJbXOX6VdTvKrhFXq6A1o 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 iQEcBAEBCAAGBQJWXHW6AAoJEDuxQgLoOKyt4YEH/2ujaHoC0o8Zz1YgeKkXXRqD ROKynCdgz/U/vM8HqginRJGXA28J75lQd/Uop4GMo2AjF/i8oMtGG7yfkAG8m9Bu SpJiwDzfnlhcwp+oeok/qAm6MGSmvm6ueP1YeWDSSAufpm/oUtfANRkCD7gv0vaF lllqx2ECdbZDaGI4ICeXrRoHYrVE3ep9DyjJP5SlWkzewtyz6MmqetU1nBCWzfFK /Lg4rxBb4tG7flAqsUnHnozQNkFVTUroXnIJF5AQM6qYs5HefYcF6U3+JEBd2gxB VIA3Wp0mSz0yvElhymMPvCN7rtHy9WjSwH1SrUj0FDEOkmT9xLda752Pl1vp0CY= =z4s8 -----END PGP SIGNATURE----- --s7tLABHKtKVjtJbXOX6VdTvKrhFXq6A1o--