From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3KKt-0005aL-Eb for qemu-devel@nongnu.org; Mon, 30 Nov 2015 04:01:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3KKn-0008RG-5E for qemu-devel@nongnu.org; Mon, 30 Nov 2015 04:01:27 -0500 Date: Mon, 30 Nov 2015 10:01:07 +0100 From: Kevin Wolf Message-ID: <20151130090107.GA4494@noname.str.redhat.com> References: <1448294400-476-1-git-send-email-kwolf@redhat.com> <1448294400-476-7-git-send-email-kwolf@redhat.com> <565899D4.1090907@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7AUc2qLy4jB3hD7Z" Content-Disposition: inline In-Reply-To: <565899D4.1090907@redhat.com> 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: Max Reitz Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org --7AUc2qLy4jB3hD7Z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 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(-) >=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, 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 >=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. > 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 not > fetch them. Before, they were included in the VMDK BDS's options, which > is not ideal but works more or less. 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). 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. I'm accepting proof of the contrary in the form of a test case. ;-) > 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). Yes, I agree. Kevin --7AUc2qLy4jB3hD7Z Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWXBBTAAoJEH8JsnLIjy/WrU0QALiLAwsAfD08uroBtb2paTbK JKalVr9kHC96yhhbV8Vyg8yt1a52lUTxeU3NIK/A6QFrH7nUrqbb5Im4JKnVYTP7 RK4I9ccF/GYk8TuBC0hlZ4XSKZIRQuNtgoD6T/yvIK+Z+xuvhS5elnqEpnkSgskD GC/TOyossFHkoaDXTV4/OORICLCI7FEK4bV3Fwz+Am4IR3bDHczE1+Fo0g7JWwZH g3/ustm61q/+eHT2SdzhscxNOhF6dg8lQYcU+w9uBvgb72KAKTRFxiCPg+3hHvrW CqZZFDbpAr7CHgLkgdh1dusRrbuNIwI8r3utTxcdflM3ekUOBfsZWBEsgwSgIqdX tbjEXI5KYuAkpbZzTIvcJBZ4WiUT1fiWxTZzRqSvnEdk8KmnOVx5ru/WPUlBdkUJ ldH0fjRAdBh7W5CnTa2QmKH+w8e5niG0XDiw7ResWzQ1wP2QhXL6rDqBUKdGvQ7p YEfrDxOe42QsRw7z9kPBjCaOGOW2yWp3Mt8mZj4i5DVpb8gnI8bPRwXgTfHbcDWh MI5yQjtxK71DR7tNEhvVh16ixq7/K6epBvvjDpzMVFWCK1i47LlgvQrfYyxK+lVr WHDAUqqaQY6ePyJzceq1IpD5852HwFWxudrU/ubWsRclwAzdGYvcGxG08sISyHww vJHBGrJNUHJL9Q/hHqTz =GmYG -----END PGP SIGNATURE----- --7AUc2qLy4jB3hD7Z--