Re: [Qemu-devel] [PATCH v2 4/6] block: add 'speed' optional parameter to block-stream

[Stefan Hajnoczi]

On Tue, Apr 24, 2012 at 4:17 PM, Eric Blake <address@hidden> wrote:
> On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
>> Allow streaming operations to be started with an initial speed limit.
>> This eliminates the window of time between starting streaming and
>> issuing block-job-set-speed.  Users should use the new optional 'speed'
>> parameter instead so that speed limits are in effect immediately when
>> the job starts.
>>
>
>> +++ b/hmp-commands.hx
>> @@ -71,8 +71,8 @@ ETEXI
>>
>>      {
>>          .name       = "block_stream",
>> -        .args_type  = "device:B,base:s?",
>> -        .params     = "device [base]",
>> +        .args_type  = "device:B,speed:o?,base:s?",
>
> Am I remembering correctly that the :o type lets me pass in suffixes,
> including 'b' to force bytes, but defaults to 'M' if I don't pass a
> suffix?  Should we be documenting the default unit and the ability to
> scale as part of the command help (see migrate_set_speed for a similar
> command that documents the scaling)?

Yes, it supports suffixes.  The HMP documentation for these commands
does not include details about the parameters, so I didn't document it
- management tools should be using QMP and HMP is mainly for
debugging/ad-hoc testing.

>> +        .params     = "device [speed] [base]",
>
> By having two optional arguments in HMP, can I write 'block_stream
> device base' and have things work, or am I now forced to provide a speed
> if I want a base?  If the latter, wouldn't this look better as:
>
> .params = "device [speed [base]]",
>
> and if so, how many other HMP commands suffer from the same
> documentation flaw?

You are right.  The .params I gave are incorrect since you must give
'speed' if you want to give 'base'.

Stefan