From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2NIL-0005ol-Lr for qemu-devel@nongnu.org; Fri, 27 Nov 2015 12:58:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a2NIK-00021v-D9 for qemu-devel@nongnu.org; Fri, 27 Nov 2015 12:58:53 -0500 References: <1448294400-476-1-git-send-email-kwolf@redhat.com> <1448294400-476-7-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <565899D4.1090907@redhat.com> Date: Fri, 27 Nov 2015 18:58:44 +0100 MIME-Version: 1.0 In-Reply-To: <1448294400-476-7-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KLlkpDFLMCf1dG4oT36g40jchMteGo3gL" 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) --KLlkpDFLMCf1dG4oT36g40jchMteGo3gL Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable 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 all= > options with "." in their name, but check for the complete prefixes of > actually existing child nodes. >=20 > 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, c= onst 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, Bl= ockDriverState *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", &chil= d_backing); > bs->open_flags &=3D ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->fi= lename); > 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, Block= DriverState *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 bad 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). 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. 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 not fetch them. Before, they were included in the VMDK BDS's options, which is not ideal but works more or less. 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). Reviewed-by: Max Reitz --KLlkpDFLMCf1dG4oT36g40jchMteGo3gL 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 iQEcBAEBCAAGBQJWWJnUAAoJEDuxQgLoOKytwc4H/isww1wMa8+Hzw+ulB7jWtIa Cw++k//Gg47/R0EidHCglFq0pIV1upjMgoorM6KYxJW70xmp6jpYjnvY/YsFySg/ ETtFK6ABfbFO9N2crAPBYAdnC88iwAwmSCrXNDLdmpcJZmSxP26JaRHXSCf0TAKa Q8p32mvhhrs1az+i74uxOFY9T3WILFl8X1UBB3qo+AkwVNhBA0qcwq8y+4wH3zw1 kcnoN1+677qMzbL+mrhItz22DSUnUh6EC3H/OXnYfzLYB9vmPhSEXtXN3GJ9v8gh nTVR5l1R/a5phLgBf8w7BZB3TWeTMGsWCSgm8PCMBYt0HT/+p1g2b59vCRpOpWo= =CRgU -----END PGP SIGNATURE----- --KLlkpDFLMCf1dG4oT36g40jchMteGo3gL--