Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo

[Eric Blake]

On 02/26/2015 06:56 AM, Alberto Garcia wrote:
> On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote:
> 
>> How about query-block-throttle, returning an array of dicts.
>>
>> => { "execute":"query-block-throttle" }
>> <= { "return": [
>>      { "name": "throttle1", "bps_max": 100000,
>>        "nodes": [ "block0", "block1" ] },
>>      { "name": "throttle2", "iops_max": 10000,
>>        "nodes": [ "block2" ] }
>>    ] }
> 
> Ok I wrote it, however I have some doubts about what to put exactly in
> the 'nodes' array.
> 
> We can just put node names as you suggest, however at the moment not
> all nodes have a name so we could end up with a list of empty strings.

At one point, we were considering a patch from Jeff Cody that guarantees
ALL nodes have a name.  Maybe that's still worthwhile.

> 
> I think the easiest solution in terms of lines of code and simplicity
> of the return type is this one:
> 
> { 'type': 'ThrottleGroupInfo',
>   'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } }

That's a bit more verbose, but also a bit more usable (all the block
data is available, rather than just a name that has to be looked up via
another command), so that could work.

> 
> { 'command': 'query-block-throttle',
>   'returns': ['ThrottleGroupInfo'] }
> 
> All the information is there, the code to fill the BlockDeviceInfo
> structure is already written so I can just make use of it.
> 
> The throttling settings themselves are not present in
> ThrottleGroupInfo, but since all nodes from a group have the same
> settings, you can get them from any of them. It also keeps the
> ThrottleGroupInfo structure simpler.
> 
> What do you think? If you're ok with this solution I can submit the
> patch series again.

Sure, sounds like it's worth that solution, since it reused code and
made for less work on your part.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature