From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acx0P-0002w0-3v for qemu-devel@nongnu.org; Mon, 07 Mar 2016 10:23:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acx0K-00070C-U5 for qemu-devel@nongnu.org; Mon, 07 Mar 2016 10:23:33 -0500 References: <1455615450-15138-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1455615450-15138-2-git-send-email-xiecl.fnst@cn.fujitsu.com> <56DB1719.50901@redhat.com> <56DD00BB.5050905@cn.fujitsu.com> From: Max Reitz Message-ID: <56DD9CE5.70303@redhat.com> Date: Mon, 7 Mar 2016 16:23:17 +0100 MIME-Version: 1.0 In-Reply-To: <56DD00BB.5050905@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qO552FPao2lCoUMdN5Mb1EJHfXRN5br9e" Subject: Re: [Qemu-devel] [PATCH v10 1/3] Add new block driver interface to add/delete a BDS's child List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Changlong Xie , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: qemu block , Jiang Yunhong , Dong Eddie , Markus Armbruster , "Dr. David Alan Gilbert" , Gonglei , zhanghailiang This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --qO552FPao2lCoUMdN5Mb1EJHfXRN5br9e Content-Type: multipart/mixed; boundary="Uc4bp9UUN2mkVmc9EgUdBo4pXUmFK1Bhs" From: Max Reitz To: Changlong Xie , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , Wen Congyang , qemu block , zhanghailiang , Gonglei Message-ID: <56DD9CE5.70303@redhat.com> Subject: Re: [PATCH v10 1/3] Add new block driver interface to add/delete a BDS's child References: <1455615450-15138-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1455615450-15138-2-git-send-email-xiecl.fnst@cn.fujitsu.com> <56DB1719.50901@redhat.com> <56DD00BB.5050905@cn.fujitsu.com> In-Reply-To: <56DD00BB.5050905@cn.fujitsu.com> --Uc4bp9UUN2mkVmc9EgUdBo4pXUmFK1Bhs Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 07.03.2016 05:16, Changlong Xie wrote: > On 03/06/2016 01:27 AM, Max Reitz wrote: >> Sorry that I wasn't so pedantic last time; or maybe I should rather be= >> sorry that I'm so pedantic this time. >=20 > Hi Max > Welcome all your comments : ) Good :-) >> On 16.02.2016 10:37, Changlong Xie wrote: >>> From: Wen Congyang >>> >>> In some cases, we want to take a quorum child offline, and take >>> another child online. >>> >>> Signed-off-by: Wen Congyang >>> Signed-off-by: zhanghailiang >>> Signed-off-by: Gonglei >>> Signed-off-by: Changlong Xie >>> Reviewed-by: Eric Blake >>> Reviewed-by: Alberto Garcia >>> --- >>> block.c | 50 >>> +++++++++++++++++++++++++++++++++++++++++++++++ >>> include/block/block.h | 5 +++++ >>> include/block/block_int.h | 5 +++++ >>> 3 files changed, 60 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index efc3c43..08aa979 100644 >>> --- a/block.c >>> +++ b/block.c [...] >>> + return; >>> + } >>> + >>> + QLIST_FOREACH(child, &parent_bs->children, next) { >>> + if (child->bs =3D=3D child_bs) { >>> + break; >>> + } >>> + } >>> + >>> + if (!child) { >>> + error_setg(errp, "The node %s is not a child of %s", >>> + bdrv_get_device_or_node_name(child_bs), >>> + bdrv_get_device_or_node_name(parent_bs)); >> >> Is there a special reason why you are using >> bdrv_get_device_or_node_name() for the child here, while in >> bdrv_add_child() you directly use the node name? >> >=20 > Although we directly use the node name in bdrv_add_child(), but we stil= l > need treat bdrv_del_child() as a separate function, right? In up > condition, we can't determine if child->bs supports BB or not, so i > think bdrv_get_device_or_node_name(child->bs) is ok here. I just realized that in order to invoke bdrv_add_child() one passes a node name to x-blockdev-change, whereas for bdrv_del_child() name of the child is passed (which is not a node name). So it makes perfect sense to always emit the node name in bdrv_add_child(), regardless of whether the BDS has a BB, because the node name was the parameter that had been given to x-blockdev-change. In contrast, the supposedly child node passed to bdrv_del_child() is not identified by its node name, so it makes sense not to limit the output to the node name but to print the BB's name if present. So indeed, this is completely fine as it is in this patch. Max >> Neither seems wrong to me. A child is unlikely to have a BB of its own= , >> but if it does, printing its name instead of the node name may be >=20 > bdrv_get_device_or_node_name() can do that. >=20 > Thanks > -Xie >=20 >> appropriate. I'm just wondering about the difference between >> bdrv_add_child() and bdrv_del_child(). >> >> Max >> >>> + return; >>> + } >>> + >>> + parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp); >>> +} >>> diff --git a/include/block/block.h b/include/block/block.h >>> index 1c4f4d8..ecde190 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs); >>> */ >>> void bdrv_drained_end(BlockDriverState *bs); >>> >>> +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *chil= d, >>> + Error **errp); >>> +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *chil= d, >>> + Error **errp); >>> + >>> #endif >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index 9ef823a..89ec138 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -305,6 +305,11 @@ struct BlockDriver { >>> */ >>> void (*bdrv_drain)(BlockDriverState *bs); >>> >>> + void (*bdrv_add_child)(BlockDriverState *parent, >>> BlockDriverState *child, >>> + Error **errp); >>> + void (*bdrv_del_child)(BlockDriverState *parent, >>> BlockDriverState *child, >>> + Error **errp); >>> + >>> QLIST_ENTRY(BlockDriver) list; >>> }; >>> >>> >> >> >=20 >=20 --Uc4bp9UUN2mkVmc9EgUdBo4pXUmFK1Bhs-- --qO552FPao2lCoUMdN5Mb1EJHfXRN5br9e 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 iQEcBAEBCAAGBQJW3ZzlAAoJEDuxQgLoOKytkmcH/RL5uwyPyoXT0yhQYBXPdu2r e03IFiJfSgYDKgcPIcIktYUYsg0ylXwWDSCiClprphAKrD2+UDXDOeRp/ba2UcNu Wdl0fse2QKD/QRGzH2bzdqCYsfzFmXuKx7o0HC5jJiUYc9IhxTq3MeKzdSmtsQTa BwZAjR91/OwbtPmzSdK/OOAPfTKm6+Yjpjc5E13XyRrn8OvcSSdFxnv0NZjNjac0 oioqJWIlliv1br+Sccf7dSb8O6CPlJoQndRJdikDAnkCz6keeyV9z4FO0IMG7mXh gSg7xvnqbfhAGnz7Q5tk35Lvu3p8fTGqSUDa0yzotShwQvbKVwMdYFdiLfgtUHk= =N2t+ -----END PGP SIGNATURE----- --qO552FPao2lCoUMdN5Mb1EJHfXRN5br9e--