Re: [Qemu-devel] [PATCH 16/16] Make qemu-io commands available in the monitor

[Kevin Wolf <kwolf@redhat.com>]

Am 28.05.2013 um 18:07 hat Eric Blake geschrieben:
> On 05/28/2013 09:27 AM, Kevin Wolf wrote:
> > The QMP version is flagged with a __org.qemu.debug- prefix in order to
> > reinforce the statement that qemu-io is for testing and debugging only,
> > with no API guarantees.
> 
> Correct use of naming conventions.
> 
> Hmm, I wonder if the recent addition of an 'abort' action to
> 'transaction' should be renamed __org.qemu.debug-abort, to make it
> obvious that it is another case of a QMP command useful mainly for
> testing, and not real-life use.

Makes sense to me.

But first I'd like to get Luiz's ack for this, because I think I'm the
first one to use an __org.qemu prefix, and I'm the first one trying to
introduce a QMP command without API stability.

> > 
> > The HMP version is simply called 'qemu-io' for convenience.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> > +++ b/hmp-commands.hx
> > @@ -1551,6 +1551,22 @@ Removes the chardev @var{id}.
> >  ETEXI
> >  
> >      {
> > +        .name       = "qemu-io",
> > +        .args_type  = "device:B,command:s",
> > +        .params     = "[device] \"[command]\"",
> > +        .help       = "run a qemu-io command on a block device",
> > +        .mhandler.cmd = hmp_qemu_io,
> > +    },
> > +
> > +STEXI
> > +@item chardev_remove id
> > +@findex chardev_remove
> > +
> > +Removes the chardev @var{id}.
> 
> Is this the right documentation?

Obviously not. I find these files where you declare monitor commands
horribly confusing, and this is the result...

> > +++ b/qapi-schema.json
> > @@ -3618,3 +3618,19 @@
> >              '*cpuid-input-ecx': 'int',
> >              'cpuid-register': 'X86CPURegister32',
> >              'features': 'int' } }
> > +
> > +##
> > +# @__org.qemu.debug-qemu-io-command
> > +#
> > +# Execute a qemu-io command
> > +#
> > +# @device:  The block device on which the command should be executed
> > +#
> > +# @command: The command to execute
> > +#
> > +# Returns:  Nothing on success
> > +#
> > +# Since: 1.6 (testing and debugging use only, no API stability)
> > +##
> > +{ 'command': '__org.qemu.debug-qemu-io-command',
> > +  'data': {'device': 'str', 'command': 'str'} }
> 
> I would have asked that 'command' be an enum, except that we promise no
> API stability so there's no need to lock us into an enum that could be
> introspected :)  This command looks fine as-is.

Wouldn't even work because the command includes parameters and things.
This is really what you would type on the qemu-io command line. So a
typical example might look like:

{ "execute": "__org.qemu.debug-qemu-io-command", "arguments":
    { "device": "ide0-hd0", "command": "write -P 0x12 4M 512k" } }

There's nothing structured about it, but for testing and debugging this
should be good enough.

> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index ffd130e..8682bea 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -2932,3 +2932,31 @@ Example:
> >  <- { "return": {} }
> >  
> >  EQMP
> > +
> > +    {
> > +        .name       = "__org.qemu.debug-qemu-io-command",
> > +        .args_type  = "device:s,command:s",
> > +        .mhandler.cmd_new = qmp_marshal_input___org_qemu_debug_qemu_io_command,
> > +    },
> > +
> > +
> > +SQMP
> > +__org.qemu.debug-qemu-io-command
> > +--------------------------------
> 
> Do we really need to have SQMP documentation of the command, or is
> listing of its handler sufficient?  In other words, if this is
> debug-only, can we just leave it undocumented instead of listing an
> example only to declare the example non-portable?

I don't mind either way. Luiz?

Kevin