Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Anthony Liguori]

On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:43:07 -0500
> Anthony Liguori<address@hidden>  wrote:
>
> > A new QMP only command to change the blockdev associated with a block device.
> > The semantics of change right now are just plain scary.  This command introduces
> > sane semantics.  For more details, see the documentation in the schema file.
>
> IMO, this has to be split into two commits. First you introduce the new command
> and then you fix do_change_block().
>
> Also, there's a small problem with the generated code: it has to return -1 on
> errors. The monitor expects a -1 return _and_ a qerror object on errors. The
> generated code in middle mode is returning 0 even on errors, which makes QMP to
> emit a UndefinedError error by default.

You mean:

    if (local_err) {
        qerror_report_err(local_err);
        error_free(local_err);
        return -1;
    }
    return 0;

That should behave exactly the right way as qerror_report_err() 
propagates sends the Error as a QError.

> > +
> > +##
> > +# @change-blockdev:
> > +#
> > +# This is the preferred interface for changing a block device.
> > +#
> > +# @device:   The block device name.
> > +#
> > +# @filename: The new filename for the block device.  This may contain options
> > +#            encoded in a format specified by @format.
> > +#
> > +# @format:   #optional The format to use open the block device
>
> We need a list (or a pointers) of valid formats.

We do, but that needs to be a command because it depends on the build 
options.

> > +#
> > +# @password: #optional The password to use if the block device is encrypted
> > +#
> > +# Returns:  Nothing on success.
> > +#          If @device is not a valid block device, DeviceNotFound
> > +#          If @format is not a valid block format, InvalidBlockFormat
> > +#          If @filename is encrypted and @password isn't supplied,
> > +#            DeviceEncrypted.  The call should be repeated with @password
> > +#            supplied.
> > +#          If @format is not specified and @filename is a format that cannot
> > +#            be safely probed, MissingParameter.
> > +#          If @filename cannot be opened, OpenFileFailed
> > +#
> > +# Since: 1.0
> > +##
> > +{ 'command': 'change-blockdev',
> > +  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
> > +           '*password': 'str'} }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index c0d0ca3..cec7135 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -121,6 +121,14 @@ EQMP
> >           .mhandler.cmd_new = do_change,
> >       },
> >
> > +    {
> > +        .name       = "change-blockdev",
> > +        .args_type  = "device:B,target:F,arg:s?pass:s?",
>
> The arguments names don't match the schema.

Do we need to set args_type for QMP?

Regards,

Anthony Liguori


> > +        .params     = "device filename [format] [password]",
> > +        .help       = "change a removable medium, optional format",
> > +        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
> > +    },
> > +
> >   SQMP
> >   change
> >   ------