Re: [Qemu-devel] [PATCH v2 09/11] block: add ability for block-stream to use node-name

[Jeff Cody]

On Wed, May 28, 2014 at 06:42:24AM -0600, Eric Blake wrote:
> On 05/27/2014 08:28 AM, Jeff Cody wrote:
> > This adds the ability for block-stream to use node-name arguments
> > for base, to specify the backing image to stream from.
> > 
> > Both 'base' and 'base-node-name' are optional, but mutually exclusive.
> > Either can be specified, but not both together.
> > 
> > The argument for "device" is now optional as well, so long as
> > base-node-name is specified.  From the node-name, we can determine
> > the full device chain.
> > 
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  blockdev.c       | 62 
> > ++++++++++++++++++++++++++++++++++++++++++++++----------
> >  hmp-commands.hx  |  2 +-
> >  hmp.c            | 10 +++++----
> >  qapi-schema.json | 15 ++++++++++----
> >  qmp-commands.hx  |  2 +-
> >  5 files changed, 70 insertions(+), 21 deletions(-)
> 
> Revisiting the HMP side of things:
> 
> > +++ b/hmp-commands.hx
> > @@ -76,7 +76,7 @@ ETEXI
> >  
> >      {
> >          .name       = "block_stream",
> > -        .args_type  = "device:B,speed:o?,base:s?",
> > +        .args_type  = "device:B?,speed:o?,base:s?,base-node-name:s?",
> 
> The HMP parser is trash.  It requires arguments to appear in declaration
> order, and in order to specify an optional argument at the end of the
> list, you must specify all earlier arguments.  Your proposed change is
> unworkable, because the ONLY way HMP can supply base-node-name is if the
> command line already supplied a base argument, but base and
> base-node-name are incompatible.
> 
> I'm not quite sure if David's idea of allowing an optional named
> argument would help:
> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg05914.html
> 
> The idea being that you'd write
>  .args_type = "device:B?,speed:o?,base-node-name:+n,base:s?"
> 
> where the command is then invoked as either:
> 
> block_stream dev 0 base    # resolve base as a file name
> 
> or as:
> 
> block_stream -n base       # resolve base as a node name, implicit dev
> block_stream dev -n base   # resolve base as a node name within dev
> 
> But you can see why I suggested earlier that maybe it's better to just
> forget about HMP entirely, and make this series focus on QMP (no need to
> stall the feature addition on the HMP warts, when backports only care
> about the QMP feature); save the HMP changes for another day (if ever).
>

I agree with not worrying about HMP in this series; the new
functionality is targeted towards a management software layer (e.g.
libvirt).  As you said, if it is desired for some reason, it can be
added later.