From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54841) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2NM0-0001vm-7s for qemu-devel@nongnu.org; Fri, 27 Nov 2015 13:02:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a2NLz-00035Q-2H for qemu-devel@nongnu.org; Fri, 27 Nov 2015 13:02:40 -0500 References: <1448294400-476-1-git-send-email-kwolf@redhat.com> <1448294400-476-7-git-send-email-kwolf@redhat.com> <565899D4.1090907@redhat.com> From: Max Reitz Message-ID: <56589AB3.5080805@redhat.com> Date: Fri, 27 Nov 2015 19:02:27 +0100 MIME-Version: 1.0 In-Reply-To: <565899D4.1090907@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="PfibqqhBbKUMieU7kTCxs3riql6bQKrP0" 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 , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --PfibqqhBbKUMieU7kTCxs3riql6bQKrP0 Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 27.11.2015 18:58, Max Reitz wrote: > 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 al= l >> options with "." in their name, but check for the complete prefixes of= >> 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(-) >=20 > Thanks, now I don't need to fix it myself. :-) >=20 > (I would have had to do that for an in-work series of mine) >=20 >> 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, B= lockDriverState *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", &chi= ld_backing); >> bs->open_flags &=3D ~BDRV_O_NO_BACKING; >> pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->f= ilename); >> 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, Bloc= kDriverState *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 >=20 > A good general solution, but I think bdrv_refresh_filename() may be bad= > enough to break with general solutions. ;-) >=20 > bdrv_refresh_filename() only considers "file" and "backing" (actually, > it only supports "file" for now, I'm working on "backing", though). The= > 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 all= =2E > The latter, however, (VMDK) does not. >=20 > This change to append_open_options results in the extent.%d options > simply being omitted altogether because bdrv_refresh_filename() does no= t > fetch them. Before, they were included in the VMDK BDS's options, which= > is not ideal but works more or less. >=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". >=20 > 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 > Reviewed-by: Max Reitz Oops, focused so much on this that I missed the issue Wen Congyang found. So this R-b is assuming that is fixed *cough*. Max --PfibqqhBbKUMieU7kTCxs3riql6bQKrP0 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 iQEcBAEBCAAGBQJWWJqzAAoJEDuxQgLoOKytpxcH/ilmy0QZ4G7P8HLm1hkU4GU+ 9nfw0CW6q6R6EJ827GMfhfzo0T/iiDNtmki0L/e8IbVsvQEAG0vFyQtvnLpEcTRh TMS1fsKrF0AopcVCS7Wwd6oRPmpu9j34/iTHfQ+J2Ay3PRZX31Qr4Pl4WlQBslEh aRfsjcfMbGDfVzf8fs7R9UnFISZH8qrdmRvAKwHqjxwgFOKAUuXCCiKS0F/kvcyC 6jDVpf+ErHBfhUV4QHK3eC1eBXqCoxTV/tAV+OcvARPSlnlfzaByYondrlp28sYb /wMo2dsQbVaWX5oS6J7ibKMEjBwKASTuMT7E+Pd1iX2rg21hLSEIdkcn9iJikZU= =MvsK -----END PGP SIGNATURE----- --PfibqqhBbKUMieU7kTCxs3riql6bQKrP0--