On Thu, 25 Aug 2011 09:21:02 -0500
Anthony Liguori <aligu...@us.ibm.com> wrote:
> On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> > On Wed, 24 Aug 2011 13:43:07 -0500
> > Anthony Liguori<aligu...@us.ibm.com> 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
> >> ------
> >
>
