Re: [Qemu-devel] [PATCH v3 2/4] fsdev: QMP interface for throttling

[Eric Blake]

On 05/04/2017 10:12 AM, Pradeep Jagadeesh wrote:

>>>>> +    IOThrottle throttle = {
>>>>> +        .has_id = true,
>>>>> +        .id = (char *) qdict_get_str(qdict, "id"),
>>>>> +        .bps = qdict_get_int(qdict, "bps"),
>>>>> +        .bps_rd = qdict_get_int(qdict, "bps_rd"),
>>>>> +        .bps_wr = qdict_get_int(qdict, "bps_wr"),
>>>>> +        .iops = qdict_get_int(qdict, "iops"),
>>>>> +        .iops_rd = qdict_get_int(qdict, "iops_rd"),
>>>>> +        .iops_wr = qdict_get_int(qdict, "iops_wr"),
>>>>
>>>> Again, don't you need to be setting .has_bps=true and so on?
>>> Same as above. I have not set any max burst values here, because I
>>> wanted to keep it in line with the block device.
>>> May be there is a room to enable these max values in both in future.
>>
>> I don't think you were getting the point of my question. Since 'bps' is
>> an optional member of struct IOThrottle, you have to set 'has_bps' at
>> the same time to make it obvious that 'bps' should affect things.
>>
> I need some more clarifications here.
> I did not understand what do you mean by an optional parameter?
> You mean I need to set "has_*" for all the parameters?

Yes, any optional parameter (one named '*foo' in the .json file) has a
counterpart has_foo boolean, which should be set to true when the
parameter is in use (and thus making it obvious when it is set to false
that it is not in use).

>> I understand that you aren't setting 'bps_max', nor it's counterpart
>> 'has_bps_max', and that's not what I was asking about.
>>
>>
>>>>> +++ b/qapi/iothrottle.json
>>>>> @@ -3,6 +3,7 @@
>>>>>  ##
>>>>>  # == QAPI IOThrottle definitions
>>>>>  ##
>>>>> +##
>>>>>  # @IOThrottle:
>>>>
>>>> This looks like a spurious change
>>> You mean the below sentence is not required?
>>
>> No, the above addition of a second ## is not required.
> If I remove that I will get the compilation error.
> I think the parser needs that.

If the parser chokes without it, then that implies patch 1 is broken,
and the fix should be squashed there for full bisectability.  Either
way, it looks like a spurious change when it is occurring in patch 3.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature