From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40669) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZePMx-0008DV-FL for qemu-devel@nongnu.org; Tue, 22 Sep 2015 11:20:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZePMw-0007qD-Io for qemu-devel@nongnu.org; Tue, 22 Sep 2015 11:20:35 -0400 References: <1442589793-7105-1-git-send-email-mreitz@redhat.com> <1442589793-7105-22-git-send-email-mreitz@redhat.com> <20150922144229.GG3999@noname.str.redhat.com> From: Max Reitz Message-ID: <560171B7.1080901@redhat.com> Date: Tue, 22 Sep 2015 17:20:23 +0200 MIME-Version: 1.0 In-Reply-To: <20150922144229.GG3999@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gQjJuIGuNwpf1FEkirs3sCPoK9qLrcIMb" Subject: Re: [Qemu-devel] [PATCH v5 21/38] block: Add blk_insert_bs() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Alberto Garcia , qemu-block@nongnu.org, John Snow , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gQjJuIGuNwpf1FEkirs3sCPoK9qLrcIMb Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 22.09.2015 16:42, Kevin Wolf wrote: > Am 18.09.2015 um 17:22 hat Max Reitz geschrieben: >> This function associates the given BlockDriverState with the given >> BlockBackend. >> >> Signed-off-by: Max Reitz >> Reviewed-by: Eric Blake >> Reviewed-by: Alberto Garcia >> --- >> block/block-backend.c | 16 ++++++++++++++++ >> include/sysemu/block-backend.h | 1 + >> 2 files changed, 17 insertions(+) >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 33145f8..652385e 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -314,6 +314,22 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBac= kend *blk) >> } >> =20 >> /* >> + * Associates a new BlockDriverState with @blk. >> + */ >> +void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs) >> +{ >> + if (bs->blk =3D=3D blk) { >> + return; >> + } >> + >> + assert(!blk->bs); >> + assert(!bs->blk); >=20 > Why is it useful to allow reconnecting a BDS to a BB it's already > connected to? I would have expected that we can assert that this is not= > the case. We can do that, too, there is no use case. It's just that I in principle don't like making things an error that do make sense and can trivially be handled gracefully. But I can see people wanting to hit an assertion as soon as something looks fishy. So I'm fine either way. I think Eric asked about the same before, so that makes two against one now. :-) >> + bdrv_ref(bs); >> + blk->bs =3D bs; >> + bs->blk =3D blk; >> +} >=20 > My series to remove bdrv_swap() introduces a blk_set_bs() function, > which looks suspiciously similar to this one, except that it allows > passing a BB that already had a BDS (it gets unrefed then) and that I > don't assert that the BDS isn't attached to a BB yet (I should do that)= =2E >=20 > Do you think that's similar enough to have only one function? Well, yours looks like blk_remove_bs()+blk_insert_bs() combined. In case we have called blk_remove_bs() before, it will effectively be the same, yes. But that difference still bothers me a bit... I'd prefer implementing blk_set_bs() by calling blk_remove_bs() and then blk_insert_bs() instead, but since these functions are not available in your bdrv_swap() series, that would prove rather difficult. I don't know. Maybe implement it separately for now, and trust that this series will stay in flight long enough for the bdrv_swap() series to get merged so I can include a commit cleaning up blk_set_bs() in this series later on? Or I can base this series on your bdrv_swap() series. Your call, as I haven't looked at it yet and thus don't know how long it'll take to get merged (albeit judging from your series in the past, it won't be too long= ). Max --gQjJuIGuNwpf1FEkirs3sCPoK9qLrcIMb 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 iQEcBAEBCAAGBQJWAXG3AAoJEDuxQgLoOKytihoH/2gTkcuYoXni8JtZyZGS1Sdf QcQ+h02KX1WErkw5xp9ReRMmVoim/KfBFEk3j21CCccdQpNo7ppfRNJs9XhUAZB8 3mRcGc3OhmORA8j169eOYePK4iTLRk1ITywTLdDtpzx72powiQR040DxdY0TSWHo 1ExJm7odf+ufmOUyJ8ZwVNBFLddz9wlapujrAtt536z3Ou0i5b6Km+G70alvxhdc Z+A6HSQtjWILfIT6gWQ48BYAdB7LYcyd38yrhrF1ZUX1rFHImY4RI/sL7XW2pMjZ NeOIRJJdPzjBzOeFBB/Fi7ZyEdUnG0k1AXQIUuGBnaAX3jPAG9Gheht27t42aQU= =C6we -----END PGP SIGNATURE----- --gQjJuIGuNwpf1FEkirs3sCPoK9qLrcIMb--