Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Eric Blake <address@hidden> writes:
>
>> On 08/10/2017 09:06 AM, Pradeep Jagadeesh wrote:
>>
>>>>> It's not "moving it back", it's keeping it where it is. But I see no big
>>>>> problem with moving it to a common file either.
>>>>
>>>> I'd rather not put every struct shared across subsystem boundaries in
>>>> its own file.
>>>>
>>>> We can keep it right where it is for now.  Bonus: more readable diff.
>>>> If we start sharing more throttle-related material than just a struct,
>>>> we can reconsider.
>>>>
>>>> We could also move it to the existing file for common stuff:
>>>> qapi/common.json.  Not a great fit, though.
>>>
>>> So, the final conclusion is to move to common.json?
>>
>> No.
>>
>> If more than one .json file would benefit by including the definition,
>> then put it in a separate file that both .json include from.
>
> This is the case.
>
> Your opinion is incompatible with mine, stated above.
>
>> But if only one .json file would be including a new file, then just
>> inline the struct directly into that one original file (in this case,
>> block-core.json) instead of creating a separate file (so no to needing
>> iothrottle.json), or putting the code in yet a different file than the
>> one that is using the struct (so no to putting it in common.json).
>
> This is no longer the case.
>
> Conclusion: no consensus, yet.

All right, let's start over and try to resolve the impasse and/or
misunderstanding.

Type BlockIOThrottle lives in qapi/block-core.json, and is used by QMP
command block_set_io_throttle.  Since 1.1.

Pradeep has a use case for throttling in fsdev.  Instead of duplicating
the relevant parts of BlockIOThrottle, qmp_block_set_io_throttle() and
hmp_block_set_io_throttle(), he factors them out smartly, into

* [PATCH 2] IOThrottle, base type of BlockIOThrottle

* [PATCH 3] throttle_set_io_limits(), called by
  qmp_block_set_io_throttle()

* [PATCH 4] hmp_initialize_io_throttle(), called by
  hmp_block_set_io_throttle()

throttle_set_io_limits() goes into existing util/throttle.c, and
hmp_initialize_io_throttle() goes into existing hmp.c.  The question is
where IOThrottle should go.

Pradeep proposes to put it in new qapi/throttle.json.  Certainly
defensible, but I really don't like putting every little thing shared
across subsystem boundaries into its own schema file.

Let me step back and discuss why we split the QAPI schema into multiple
files in the first place.  For me, the one and only reason is
MAINTAINERS.

If the block folks should continue to maintain IOThrottle, then it
should stay put in block-core.json.

If somebody else should start maintaining it, it should move.  We'd need
a suitable entry in MAINTAINERS then.

I don't see why maintenance should change, and therefore believe it
should stay put.

Eric?