Re: [Qemu-devel] [PULL 33/44] block: New option to define the intervals for collecting I/O statistics

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/10/2015 07:14 AM, Stefan Hajnoczi wrote:
>> From: Alberto Garcia <address@hidden>
>> 
>> The BlockAcctStats structure contains a list of BlockAcctTimedStats.
>> Each one of these collects statistics about the minimum, maximum and
>> average latencies of all I/O operations in a certain interval of time.
>> 
>> This patch adds a new "stats-intervals" option that allows defining
>> these intervals.
>> 
>> Signed-off-by: Alberto Garcia <address@hidden>
>> Message-id: address@hidden
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  blockdev.c           | 37 +++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json |  4 ++++
>>  2 files changed, 41 insertions(+)
>
>> +++ b/qapi/block-core.json
>> @@ -1503,6 +1503,9 @@
>>  # @stats-account-failed: #optional whether to include failed
>>  #                         operations when computing latency and last
>>  #                         access statistics (default: true) (Since 2.5)
>> +# @stats-intervals: #optional colon-separated list of intervals for
>> +#                   collecting I/O statistics, in seconds (default: none)
>> +#                   (Since 2.5)
>
> Eww. Sorry for not noticing this sooner, but can we please fix this to be:
>
> '*stats-intervals':['int']
>
> Having to post-process parse for colons means that the JSON interface
> was not properly defined.

Basic QMP rule: never encode in strings when a natural JSON encoding
exists.

If you want a fancy string encoding for HMP or command line, use
suitable visitors.  If I remember correctly, NumaNodeOptions member cpus
can serve as example for a list of integers.  Suggest to start at
parse_numa().

> I'm okay if the fix is a followup, but we need to get it in before 2.5
> bakes in the gross interface.

For 2.5, either fix it, revert it, or rename it to x-.  It must not
become ABI in this state.

>>  # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>>  #                 (default: off)
>>  #
>> @@ -1520,6 +1523,7 @@
>>              '*read-only': 'bool',
>>              '*stats-account-invalid': 'bool',
>>              '*stats-account-failed': 'bool',
>> +            '*stats-intervals': 'str',
>>              '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
>>  
>>  ##
>>