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

[Luiz Capitulino]

On Thu, 25 Aug 2011 09:21:02 -0500
Anthony Liguori <address@hidden> wrote:

> 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.

Oh that's right, what's happening is that qerror_report_err() is returning
before setting qerror in the Monitor object... I reported that in my
review of patch 01/14.

> 
> >> +
> >> +##
> >> +# @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?

Yes.

> 
> 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
> >>   ------
> >
>