From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akyiu-00048N-5N for qemu-devel@nongnu.org; Tue, 29 Mar 2016 14:50:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akyit-0000n8-3S for qemu-devel@nongnu.org; Tue, 29 Mar 2016 14:50:40 -0400 References: <1458675397-24956-1-git-send-email-kwolf@redhat.com> <1458675397-24956-8-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <56FACE74.7000708@redhat.com> Date: Tue, 29 Mar 2016 20:50:28 +0200 MIME-Version: 1.0 In-Reply-To: <1458675397-24956-8-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="U5F8rw2rtrQWqLEuagruQ06Ec1R7qg6Ll" Subject: Re: [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next() 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) --U5F8rw2rtrQWqLEuagruQ06Ec1R7qg6Ll Content-Type: multipart/mixed; boundary="pttdh67b1l76mpvfJ7x8JUFf0GhOXRuo4" From: Max Reitz To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org Message-ID: <56FACE74.7000708@redhat.com> Subject: Re: [PATCH 7/9] block: Avoid bs->blk in bdrv_next() References: <1458675397-24956-1-git-send-email-kwolf@redhat.com> <1458675397-24956-8-git-send-email-kwolf@redhat.com> In-Reply-To: <1458675397-24956-8-git-send-email-kwolf@redhat.com> --pttdh67b1l76mpvfJ7x8JUFf0GhOXRuo4 Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 22.03.2016 20:36, Kevin Wolf wrote: > We need to introduce a separate BdrvNextIterator struct that can keep > more state than just the current BDS in order to avoid using the bs->bl= k > pointer. >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 34 +++++++++----------------------- > block/block-backend.c | 44 +++++++++++++++++++++++++++-------= -------- > block/io.c | 13 +++++++------ > block/snapshot.c | 30 ++++++++++++++++------------ > blockdev.c | 3 ++- > include/block/block.h | 3 ++- > include/sysemu/block-backend.h | 1 - > migration/block.c | 4 +++- > monitor.c | 6 ++++-- > qmp.c | 5 ++++- > 10 files changed, 77 insertions(+), 66 deletions(-) The connection with patch 5 is a bit funny. I see why you have two patches for this issue (because the first one introduces bdrv_has_blk()), but I think it would be better to clear up their relationship in the first patch's commit message. [...] > diff --git a/block/block-backend.c b/block/block-backend.c > index fdd1727..b71b822 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -325,28 +325,40 @@ BlockBackend *blk_next(BlockBackend *blk) > : QTAILQ_FIRST(&monitor_block_backends); > } > =20 > -/* > - * Iterates over all BlockDriverStates which are attached to a BlockBa= ckend. > - * This function is for use by bdrv_next(). > - * > - * @bs must be NULL or a BDS that is attached to a BB. > - */ > -BlockDriverState *blk_next_root_bs(BlockDriverState *bs) > -{ > +struct BdrvNextIterator { > + int phase; A verbose enum would have been nicer. > BlockBackend *blk; > + BlockDriverState *bs; > +}; > =20 > - if (bs) { > - assert(bs->blk); > - blk =3D bs->blk; > - } else { > - blk =3D NULL; > +/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are o= wned by > + * the monitor or attached to a BlockBackend */ > +BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **b= s) > +{ > + if (!it) { > + it =3D g_new0(BdrvNextIterator, 1); > + } > + > + if (it->phase =3D=3D 0) { > + do { > + it->blk =3D blk_all_next(it->blk); > + *bs =3D it->blk ? blk_bs(it->blk) : NULL; > + } while (it->blk && *bs =3D=3D NULL); This will be interesting with multiple BBs per BDS. I guess we can do something like only take the BDS if the BB is the first one in its parent list. > + > + if (*bs) { > + return it; :-) > + } > + it->phase =3D 1; > } > =20 > + /* Ignore all BDSs that are attached to a BlockBackend here; they = have been > + * handled by the above block already */ > do { > - blk =3D blk_all_next(blk); > - } while (blk && !blk->root); > + it->bs =3D bdrv_next_monitor_owned(it->bs); > + *bs =3D it->bs; > + } while (*bs && bdrv_has_blk(*bs)); > =20 > - return blk ? blk->root->bs : NULL; > + return *bs ? it : NULL; > } > =20 > /* Reviewed-by: Max Reitz [...] --pttdh67b1l76mpvfJ7x8JUFf0GhOXRuo4-- --U5F8rw2rtrQWqLEuagruQ06Ec1R7qg6Ll 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 iQEcBAEBCAAGBQJW+s50AAoJEDuxQgLoOKyt194H/jaHmF8HaA10XfezBhQLTQAc nzoZ7pwxxFvYB+wIt0U79kyCGuW97aIcBRzNcEEA5EAZM9Vgp2fgaafwW9AqPZC3 aMJ1ely5OLCdmbTjLv0s/l+qxH/AZZgM3/emTjk+d0aAl90/RQnYrfrpmPwsUPBV 4q6VaVYtMkp6ajEEZXfHylGv5aX+euCW9R4KzmKZqKuAcisHiiMdNlziRGty0BT3 7eUa9Ys/hHcBlUJK5oIYeuJvoqXF4gIGntJERwom4T5HfVdoSfb95ynqKPXxF6zK 8nB78XIAB3mY8nlKoXvVEJTn4u9BRuwt34HGvh8+Ja/xaxy/4NUuJt1PGStAfcc= =5WqZ -----END PGP SIGNATURE----- --U5F8rw2rtrQWqLEuagruQ06Ec1R7qg6Ll--