Re: [Qemu-block] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Kevin Wolf]

Am 15.09.2015 um 11:20 hat Kevin Wolf geschrieben:
> Am 15.09.2015 um 10:56 hat Alberto Garcia geschrieben:
> > On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <address@hidden> wrote:
> > 
> > >> +{ 'command': 'x-child-add',
> > >> +  'data' : { 'parent': 'str', 'child': 'str' } }
> > >
> > > This is probably not future-proof and only made for the special case
> > > of quorum. Specifically, one thing I'm missing is some way to specfiy
> > > what kind of child the new node is when a node can take different
> > > types of children (e.g. bs->file and bs->backing_hd).
> > 
> > Children here are specified by child roles, aren't they?
> 
> Possibly, but actually, currently I'm inclined to think that they
> aren't. Child roles are still relatively new, though, so it's hard to
> say if this is just because we didn't use them in that way so far, or
> because they are really the wrong tool.
> 
> What I can say is that traditionally children are identified by option
> names. Block drivers assign a ChildRole when processing the option, and
> both are equivalent only if the same ChildRole is never used for two
> different children. I believe that that's currently true, but I'm
> doubtful whether it will remain this way (even looking at blkverify,
> which has one &child_file and one &child_format, things start to look a
> bit confusing).

What I wanted to mention, but forgot to do, is that obviously we could
use more specific ChildRoles than &child_file and &child_format,
essentially creating one ChildRole per option and making them equivalent
this way.

I'm not sure if that would be a good or a bad idea.

> Possibly the right and consistent way to change the set of children
> would be through a QMP command exposing bdrv_reopen(), which would also
> be used for changing other options at runtime.
> 
> My current pull request adds the qemu-io (and therefore HMP) way of
> doing this, but finding a good QMP interface will still be a challenge.
> 
> > We could have a ChildRole enum with 'file', 'format' and 'backing' that
> > would be passed to this command and use the same enumeration in
> > bdrv_open_image() rather than a pointer to BdrvChildRole.
> 
> Yes, that would be an option if ChildRole turned out to be enough.
> 
> Kevin
>