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

[Anthony Liguori <aliguori@us.ibm.com>]

On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:43:07 -0500
> Anthony Liguori<aliguori@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.

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