qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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;
>>   };
>>
>>
>
>

  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).