Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the ThrottleLimits member names

From: Eric Blake <eblake@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, xiezhide <xiezhide@huawei.com>
Cc: berto@igalia.com, qemu-devel@nongnu.org, groug@kaod.org,
	zengcanfu@huawei.com, aneesh.kumar@linux.vnet.ibm.com,
	jinxuefeng@huawei.com, chenhui.rtos@huawei.com
Subject: Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the ThrottleLimits member names
Date: Wed, 28 Nov 2018 07:09:05 -0600	[thread overview]
Message-ID: <1a77a48a-7160-8282-7c8f-de0b6dfe68f3@redhat.com> (raw)
In-Reply-To: <87ftvlh7us.fsf@dusky.pond.sub.org>

On 11/28/18 3:25 AM, Markus Armbruster wrote:
> xiezhide <xiezhide@huawei.com> writes:
> 
>> Rename the ThrottleLimits member names and modify related code
>>
>> Signed-off-by: xiezhide <xiezhide@huawei.com>
>> ---
>>   qapi/block-core.json |  70 +++++++++++-----------
>>   util/throttle.c      | 163 +++++++++++++++++++++++++--------------------------
>>   2 files changed, 116 insertions(+), 117 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index d4fe710..4ffaaea 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json

>>   ##
>>   { 'struct': 'ThrottleLimits',
>> -  'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',

>> +  'data': { '*iops' : 'int', '*iops_max' : 'int',
>> +            '*iops_max_length' : 'int', '*iops_rd' : 'int',
>> +            '*iops_rd_max' : 'int', '*iops_rd_max_length' : 'int',
>> +            '*iops_wr' : 'int', '*iops_wr_max' : 'int',
>> +            '*iops_wr_max_length' : 'int', '*bps' : 'int',
>> +            '*bps_max' : 'int', '*bps_max_length' : 'int',
>> +            '*bps_rd' : 'int', '*bps_rd_max' : 'int',
>> +            '*bps_rd_max_length' : 'int', '*bps_wr' : 'int',
>> +            '*bps_wr_max' : 'int', '*bps_wr_max_length' : 'int',
>> +            '*iops_size' : 'int' } }
> 
> Compatibility break.  Why is that okay?

Grepping qapi/qapi-introspection.c shows 0 hits for either 
ThrottleLimits or for iops-total, so there are no QMP commands affected. 
There might, however, be command line and/or QOM paths affected, which 
is harder to audit since those don't affect instrospection.

> 
> Even if it is, you still run afoul of docs/devel/qapi-code-gen.txt:
> 
>      Command names, and member names within a type, should be all lower
>      case with words separated by a hyphen.  However, some existing older
>      commands and complex types use underscore; when extending such
>      expressions, consistency is preferred over blindly avoiding
>      underscore.
> 
> The exception doesn't apply here.

Ah, but it does, because we are refactoring code to share a common QAPI 
struct in a later patch, where we need this exact naming to avoid 
breaking that command.

So the REAL problem with this commit is that the commit message does not 
give enough details, either why this is safe (because it does not impact 
existing QMP commands) or needed (because we will be using it to rewrite 
an existing QMP command that needs this spelling).

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