Re: [Qemu-devel] [Patch for-2.5 v2 3/6] Add new block driver interface to add/delete a BDS's child

From: Eric Blake <eblake@redhat.com>
To: Wen Congyang <wency@cn.fujitsu.com>,
	qemu devel <qemu-devel@nongnu.org>,
	Markus Armbruster <armbru@redhat.com>,
	Alberto Garcia <berto@igalia.com>,
	Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	zhanghailiang <zhang.zhanghailiang@huawei.com>,
	qemu block <qemu-block@nongnu.org>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Dong Eddie <eddie.dong@intel.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	Gonglei <arei.gonglei@huawei.com>,
	Yang Hongyang <yanghy@cn.fujitsu.com>
Subject: Re: [Qemu-devel] [Patch for-2.5 v2 3/6] Add new block driver interface to add/delete a BDS's child
Date: Tue, 1 Sep 2015 09:30:56 -0600	[thread overview]
Message-ID: <55E5C4B0.1080301@redhat.com> (raw)
In-Reply-To: <55E4F4FF.1000504@cn.fujitsu.com>

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On 08/31/2015 06:44 PM, Wen Congyang wrote:

>>
>>> + * 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 *bs, QDict *options, Error **errp)
>>> +{
>>> +
>>> +    if (!bs->drv || !bs->drv->bdrv_add_child) {
>>> +        error_setg(errp, "The BDS %s doesn't support adding a child",
>>> +                   bdrv_get_device_or_node_name(bs));
>>> +        return;
>>> +    }
>>> +
>>> +    bs->drv->bdrv_add_child(bs, options, errp);
>>
>> Should this also check that bs is not already a child of something?  Or
>> a bit looser, we may want to allow a BDS to be a child of multiple trees
>> (a common shared backing file), but we still definitely don't want to
>> allow nonsensical loops such as trying to make a BDS be hot-added as its
>> own child.
>>
> 
> hot-added BDS is a new BDS, but it is OK to check it here. I will update it
> in the next version.

Design-wise, I think we really want to have the add-child operation be
handed a pre-opened BDS, rather than the options dictionary to open the
BDS itself.  That is, we should use the existing blockdev-add (and
enhance it to support everything) to open the BDS, and then this command
should just attach that BDS as the new child (which is why it IS
important that we validate that the new BDS being added doesn't create
an invalid loop).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]