From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58886) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acmZs-0004iG-HS for qemu-devel@nongnu.org; Sun, 06 Mar 2016 23:15:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acmZr-0003uX-13 for qemu-devel@nongnu.org; Sun, 06 Mar 2016 23:15:28 -0500 Message-ID: <56DD00BB.5050905@cn.fujitsu.com> Date: Mon, 7 Mar 2016 12:16:59 +0800 From: Changlong Xie MIME-Version: 1.0 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> In-Reply-To: <56DB1719.50901@redhat.com> Content-Type: text/plain; charset="iso-8859-15"; format=flowed Content-Transfer-Encoding: 7bit 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: Max Reitz , 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 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. Hi Max Welcome all your comments : ) > > 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 >> @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs) >> QDECREF(json); >> } >> } >> + >> +/* >> + * Hot add/remove a BDS's child. So the user can take a child offline when >> + * it is broken and take a new child online >> + */ >> +void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, >> + Error **errp) >> +{ >> + >> + if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) { >> + error_setg(errp, "The node %s doesn't support adding a child", > > As I said in my reply to v9's patch 3 (and I see you've followed it), I > don't quite like contractions in error messages, so I'd rather like this > to be "does not" instead of "doesn't". Okay > > If you don't decide to change this patch, then feel free to leave this > as it is, because that way you can keep Eric's and Berto's R-bs. > >> + bdrv_get_device_or_node_name(parent_bs)); >> + return; >> + } >> + >> + if (!QLIST_EMPTY(&child_bs->parents)) { >> + error_setg(errp, "The node %s already has a parent", >> + child_bs->node_name); >> + return; >> + } >> + >> + parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp); >> +} >> + >> +void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, >> + Error **errp) > > I wondered before and now I'm wondering why I didn't say anything. Why > is this function taking a BDS pointer as child_bs? Why not just > "BdrvChild *child"? Its only caller will be qmp_x_blockdev_change() > which gets the child BDS from bdrv_find_child(), which could just as > well return a BdrvChild instead of a BDS pointer. > > I see two advantages to this: First, it's a bit clearer since this > operation actually does not delete the child *BDS* but only the *edge* > between parent and child, and that edge is what a BdrvChild is. That's convincing. > > And second, some block drivers may prefer the BdrvChild over the BDS > itself. They can always trivially get the BDS from the BdrvChild > structure, but the other way around is difficult. > > The only disadvantage I can see is that it then becomes asymmetric to > bdrv_add_child(). But that's fine, it's a different function after all. As you said, they are different fuctions at all. So i don't think it's a big deal. > bdrv_add_child() creates a BdrvChild object (an edge), bdrv_del_child() > deletes it. > > (I don't know whether you had this discussion with someone else before, > though. Sorry if you did, I missed it, then.) > >> +{ >> + BdrvChild *child; >> + >> + if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) { >> + error_setg(errp, "The node %s doesn't support removing a child", >> + bdrv_get_device_or_node_name(parent_bs)); > > Again, optional s/doesn't/does not/. okay > >> + return; >> + } >> + >> + QLIST_FOREACH(child, &parent_bs->children, next) { >> + if (child->bs == 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? > Although we directly use the node name in bdrv_add_child(), but we still 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. > 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 bdrv_get_device_or_node_name() can do that. Thanks -Xie > 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 *child, >> + Error **errp); >> +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child, >> + 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; >> }; >> >> > >