From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40230) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akxZR-000653-6g for qemu-devel@nongnu.org; Tue, 29 Mar 2016 13:36:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akxZM-0005X5-SJ for qemu-devel@nongnu.org; Tue, 29 Mar 2016 13:36:49 -0400 References: <1458675397-24956-1-git-send-email-kwolf@redhat.com> <1458675397-24956-2-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <56FABD1F.9030409@redhat.com> Date: Tue, 29 Mar 2016 19:36:31 +0200 MIME-Version: 1.0 In-Reply-To: <1458675397-24956-2-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="p6wWvcsE4NKCPHBDJxh25oBuwRaTRpbTI" Subject: Re: [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize 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) --p6wWvcsE4NKCPHBDJxh25oBuwRaTRpbTI Content-Type: multipart/mixed; boundary="jHB0PHEGCX2ot6LDNlkFC9d5SEcomP70c" From: Max Reitz To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org Message-ID: <56FABD1F.9030409@redhat.com> Subject: Re: [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize References: <1458675397-24956-1-git-send-email-kwolf@redhat.com> <1458675397-24956-2-git-send-email-kwolf@redhat.com> In-Reply-To: <1458675397-24956-2-git-send-email-kwolf@redhat.com> --jHB0PHEGCX2ot6LDNlkFC9d5SEcomP70c Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 22.03.2016 20:36, Kevin Wolf wrote: > We want to get rid of BlockDriverState.blk in order to allow multiple > BlockBackends per BDS. Converting the device callbacks in block.c (whic= h > assume a single BlockBackend) to per-child callbacks gets us rid of the= > first few instances. >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 38 +++++++++++++++++++++++++------------- > block/block-backend.c | 15 ++++++++++++++- > include/block/block_int.h | 4 +++- > 3 files changed, 42 insertions(+), 15 deletions(-) >=20 > diff --git a/block.c b/block.c > index fb37b91..1fb5dac 100644 > --- a/block.c > +++ b/block.c > @@ -1216,6 +1216,27 @@ void bdrv_unref_child(BlockDriverState *parent, = BdrvChild *child) > bdrv_root_unref_child(child); > } > =20 > + > +static void bdrv_parent_cb_change_media(BlockDriverState *bs, bool loa= d) > +{ > + BdrvChild *c; > + QLIST_FOREACH(c, &bs->parents, next_parent) { > + if (c->role->change_media) { > + c->role->change_media(c, load); > + } > + } > +} > + > +static void bdrv_parent_cb_resize(BlockDriverState *bs) > +{ > + BdrvChild *c; > + QLIST_FOREACH(c, &bs->parents, next_parent) { > + if (c->role->resize) { > + c->role->resize(c); > + } > + } > +} > + > /* > * Sets the backing file link of a BDS. A new reference is created; ca= llers > * which don't need their own reference any more must call bdrv_unref(= ). > @@ -1674,9 +1695,7 @@ static int bdrv_open_inherit(BlockDriverState **p= bs, const char *filename, > } > =20 > if (!bdrv_key_required(bs)) { > - if (bs->blk) { > - blk_dev_change_media_cb(bs->blk, true); > - } > + bdrv_parent_cb_change_media(bs, true); > } else if (!runstate_check(RUN_STATE_PRELAUNCH) > && !runstate_check(RUN_STATE_INMIGRATE) > && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ > @@ -2122,9 +2141,7 @@ static void bdrv_close(BlockDriverState *bs) > bdrv_release_named_dirty_bitmaps(bs); > assert(QLIST_EMPTY(&bs->dirty_bitmaps)); > =20 > - if (bs->blk) { > - blk_dev_change_media_cb(bs->blk, false); > - } > + bdrv_parent_cb_change_media(bs, false); This is probably unnecessary altogether, but I'm certain about it after http://lists.nongnu.org/archive/html/qemu-block/2015-11/msg00349.html. So I'll take a note of removing this then. > if (bs->drv) { > BdrvChild *child, *next; > @@ -2605,9 +2622,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t o= ffset) > if (ret =3D=3D 0) { > ret =3D refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);= > bdrv_dirty_bitmap_truncate(bs); > - if (bs->blk) { > - blk_dev_resize_cb(bs->blk); > - } > + bdrv_parent_cb_resize(bs); > } > return ret; > } > @@ -2718,10 +2733,7 @@ int bdrv_set_key(BlockDriverState *bs, const cha= r *key) > bs->valid_key =3D 0; > } else if (!bs->valid_key) { > bs->valid_key =3D 1; > - if (bs->blk) { > - /* call the change callback now, we skipped it on open */ > - blk_dev_change_media_cb(bs->blk, true); > - } > + bdrv_parent_cb_change_media(bs, true); Not sure why you removed the comment here. In case you send a v2, I'd keep it. Whichever way you decide: Reviewed-by: Max Reitz > } > return ret; > } > diff --git a/block/block-backend.c b/block/block-backend.c > index 8b4eb1a..9d973ba 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -92,9 +92,15 @@ static void blk_root_inherit_options(int *child_flag= s, QDict *child_options, > } > static bool blk_drain_throttling_queue(BdrvChild *child); > =20 > +static void blk_root_change_media(BdrvChild *child, bool load); > +static void blk_root_resize(BdrvChild *child); > + > static const BdrvChildRole child_root =3D { > .inherit_options =3D blk_root_inherit_options, > =20 > + .change_media =3D blk_root_change_media, > + .resize =3D blk_root_resize, > + > .drain_queue =3D blk_drain_throttling_queue, > }; > =20 > @@ -553,6 +559,11 @@ void blk_dev_change_media_cb(BlockBackend *blk, bo= ol load) > } > } > =20 > +static void blk_root_change_media(BdrvChild *child, bool load) > +{ > + blk_dev_change_media_cb(child->opaque, load); > +} > + > /* > * Does @blk's attached device model have removable media? > * %true if no device model is attached. > @@ -607,8 +618,10 @@ bool blk_dev_is_medium_locked(BlockBackend *blk) > /* > * Notify @blk's attached device model of a backend size change. > */ > -void blk_dev_resize_cb(BlockBackend *blk) > +static void blk_root_resize(BdrvChild *child) > { > + BlockBackend *blk =3D child->opaque; > + > if (blk->dev_ops && blk->dev_ops->resize_cb) { > blk->dev_ops->resize_cb(blk->dev_opaque); > } > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 000d2c0..f60cb7c 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -356,6 +356,9 @@ struct BdrvChildRole { > void (*inherit_options)(int *child_flags, QDict *child_options, > int parent_flags, QDict *parent_options); > =20 > + void (*change_media)(BdrvChild *child, bool load); > + void (*resize)(BdrvChild *child); > + > bool (*drain_queue)(BdrvChild *child); > }; > =20 > @@ -695,7 +698,6 @@ bool blk_dev_has_tray(BlockBackend *blk); > void blk_dev_eject_request(BlockBackend *blk, bool force); > bool blk_dev_is_tray_open(BlockBackend *blk); > bool blk_dev_is_medium_locked(BlockBackend *blk); > -void blk_dev_resize_cb(BlockBackend *blk); > =20 > void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_s= ectors); > bool bdrv_requests_pending(BlockDriverState *bs); >=20 --jHB0PHEGCX2ot6LDNlkFC9d5SEcomP70c-- --p6wWvcsE4NKCPHBDJxh25oBuwRaTRpbTI 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+r0fAAoJEDuxQgLoOKytsBsH/id+RRADwOPc5GGn+qGlvLoX LFajFNZAAXz04LoejlsTkXFNlUbIJWae0DAFHD0iqsSPqYMmN+z1mlQ2QpqRDRib PokoWdcYpWoRwKoqHjZkm2Rf4EJn8xoMrNrqTbPlnCJVhHFPYKKQ8kgptHIXIrgg JiFsdJEScfIR8w/a46l2YmESESgonxUvQlwvAmsoH3V4IxiLj0VKQb7ErMRtgdaB wGx3tBdIKEe7NFpsDJEE0rwDzpM5QUQ4+ubwJ9mEcvFdLk82SxQRGvr7rW6DTEvd ID1YkMD6Y5TtR07OoBbcEg28ygfCfP8kXaWPaNR1TlnM4MSE16ygX9lnIbZ33tQ= =DWAh -----END PGP SIGNATURE----- --p6wWvcsE4NKCPHBDJxh25oBuwRaTRpbTI--