Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com>  wrote:
>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>>>
>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>>   wrote:
>>>>>>
>>>>>> Index: qemu/hmp-commands.hx
>>>>>> ===================================================================
>>>>>> --- qemu.orig/hmp-commands.hx
>>>>>> +++ qemu/hmp-commands.hx
>>>>>> @@ -70,6 +70,20 @@ but should be used with extreme caution.
>>>>>>   resizes image files, it can not resize block devices like LVM volumes.
>>>>>>   ETEXI
>>>>>>
>>>>>> +    {
>>>>>> +        .name       = "block_set",
>>>>>> +        .args_type  = "device:B,device:O",
>>>>>> +        .params     = "device [prop=value][,...]",
>>>>>> +        .help       = "Change block device parameters
>>>>>> [hostcache=on/off]",
>>>>>> +        .user_print = monitor_user_noop,
>>>>>> +        .mhandler.cmd_new = do_block_set,
>>>>>> +    },
>>>>>> +STEXI
>>>>>> +@item block_set @var{config}
>>>>>> +@findex block_set
>>>>>> +Change block device parameters (eg: hostcache=on/off) while guest is
>>>>>> running.
>>>>>> +ETEXI
>>>>>> +
>>>>>
>>>>> block_set_hostcache() please.
>>>>>
>>>>> Multiplexing commands is generally a bad idea.  It weakens typing.  In
>>>>> the
>>>>> absence of a generic way to set block device properties, implementing
>>>>> properties as generic in the QMP layer seems like a bad idea to me.
>>>>
>>>> The idea behind block_set was to have a unified interface for changing
>>>> block device parameters at runtime.  This prevents us from reinventing
>>>> new commands from scratch.  For example, block I/O throttling is
>>>> already queued up to add run-time parameters.
>>>>
>>>> Without a unified command we have a bulkier QMP/HMP interface,
>>>> duplicated code, and possibly inconsistencies in syntax between the
>>>> commands.  Isn't the best way to avoid these problems a unified
>>>> interface?
>>>>
>>>> I understand the lack of type safety concern but in this case we
>>>> already have to manually pull parsed arguments (i.e. cast to specific
>>>> types and deal with invalid input).  To me this is a reason *for*
>>>> using a unified interface like block_set.
>>>
>>> Think about it from a client perspective.  How do I determine which
>>> properties are supported by this version of QEMU?  I have no way to identify
>>> programmatically what arguments are valid for block_set.
>>>
>>> OTOH, if you have strong types like block_set_hostcache, query-commands
>>> tells me exactly what's supported.
>>
>> Use query-block and see if 'hostcache' is there.  If yes, then the
>> hostcache parameter is available.  If we allow BlockDrivers to have
>> their own runtime parameters then query-commands does not tell you
>> anything because the specific BlockDriver may or may not support that
>> runtime parameter - you need to use query-block.
>
> Let's reach agreement here.  The choices are:
>
> 1. Top-level block_set command.  Supported parameters are discovered
> by looking query-block output.

I'm strongly opposed to this.  There needs to be a single consistent way 
to determine supported operations with QMP.

And that single mechanism already exists--query_commands.

> 2. Top-level command for each parameter (e.g. block_set_hostcache).
> Supported parameters are easily discoverable via query-commands.  If
> individual block devices support different sets of parameters then
> they may have to return -ENOTSUPP.
>
> I like the block_set approach.
>
> Anthony, Kevin, Supriya: Any thoughts?

For the sake of overall QMP sanity, I think block_set_hostcache is 
really our only option.

Regards,

Anthony Liguori

> Stefan
>