From: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
To: Max Reitz <mreitz@redhat.com>, qemu devel <qemu-devel@nongnu.org>,
Eric Blake <eblake@redhat.com>, Alberto Garcia <berto@igalia.com>,
Kevin Wolf <kwolf@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>
Cc: qemu block <qemu-block@nongnu.org>,
Jiang Yunhong <yunhong.jiang@intel.com>,
Dong Eddie <eddie.dong@intel.com>,
Markus Armbruster <armbru@redhat.com>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
Gonglei <arei.gonglei@huawei.com>,
zhanghailiang <zhang.zhanghailiang@huawei.com>
Subject: Re: [Qemu-devel] [PATCH v10 1/3] Add new block driver interface to add/delete a BDS's child
Date: Mon, 7 Mar 2016 12:16:59 +0800 [thread overview]
Message-ID: <56DD00BB.5050905@cn.fujitsu.com> (raw)
In-Reply-To: <56DB1719.50901@redhat.com>
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 <wency@cn.fujitsu.com>
>>
>> In some cases, we want to take a quorum child offline, and take
>> another child online.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>> 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;
>> };
>>
>>
>
>
next prev parent reply other threads:[~2016-03-07 4:15 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-16 9:37 [Qemu-devel] [PATCH v10 0/3] qapi: child add/delete support Changlong Xie
2016-02-16 9:37 ` [Qemu-devel] [PATCH v10 1/3] Add new block driver interface to add/delete a BDS's child Changlong Xie
2016-03-05 17:27 ` Max Reitz
2016-03-07 4:16 ` Changlong Xie [this message]
2016-03-07 15:23 ` Max Reitz
2016-02-16 9:37 ` [Qemu-devel] [PATCH v10 2/3] quorum: implement bdrv_add_child() and bdrv_del_child() Changlong Xie
2016-03-05 18:13 ` Max Reitz
2016-03-07 9:13 ` Changlong Xie
2016-03-07 16:02 ` Eric Blake
2016-03-07 16:02 ` Max Reitz
2016-03-08 2:57 ` Changlong Xie
2016-03-09 15:27 ` Max Reitz
2016-03-08 1:42 ` Changlong Xie
2016-02-16 9:37 ` [Qemu-devel] [PATCH v10 3/3] qmp: add monitor command to add/remove a child Changlong Xie
2016-03-05 18:33 ` Max Reitz
2016-03-07 9:15 ` Changlong Xie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56DD00BB.5050905@cn.fujitsu.com \
--to=xiecl.fnst@cn.fujitsu.com \
--cc=arei.gonglei@huawei.com \
--cc=armbru@redhat.com \
--cc=berto@igalia.com \
--cc=dgilbert@redhat.com \
--cc=eblake@redhat.com \
--cc=eddie.dong@intel.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=yunhong.jiang@intel.com \
--cc=zhang.zhanghailiang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).