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

[xiezhide]

> 
> Eric Blake <address@hidden> writes:
> 
> > On 11/28/18 3:25 AM, Markus Armbruster wrote:
> >> xiezhide <address@hidden> writes:
> >>
> >>> Rename the ThrottleLimits member names and modify related code
> >>>
> >>> Signed-off-by: xiezhide <address@hidden>
> >>> ---
> >>>   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.
> 
> I see.
> 
> >           There might, however, be command line and/or QOM paths
> > affected, which is harder to audit since those don't affect
> > instrospection.
> 
> Yet another argument for QAPIfication...
> 
> Let's check.  git-grep ThrottleLimits finds:
> 
> * block/throttle-groups.c: getter and setter for QOM class
>   "throttle-group" property "limits".  This class is user-creatable.
>   Therefore, this is an externally visible interface.
> 
>   The question is whether it's a *stable* interface.  The status of QOM
>   properties in that regard is not clear to me.  If I remember
>   correctly, we treated it as "just for debugging and such" at least
>   initially.  But is that still a defensible position?  It ceases to be
>   one as soon as you need QOM properties to do something users ought to
>   be able to do.  Like configuring stuff that is meant to be configured.
>   If that's the case, we need to examine the situation, and clarify our
>   ABI promises.  Recommend a separate thread.
> 
> * util/throttle.c: Functions to convert to and from ThrottleConfig.  The
>   conversion to ThrottleConfig uses hardcoded member names in error
>   messages.  Relevant only insofar we need to remember to change them
>   when we rename members.
Yes, find few hard coded member names in error message need to change.
Will fix it in new version

 
> >> 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).
> 
> Yes, the commit message needs to make explain why the change is useful, and
> why it is safe.


Strongly endorse , will with clear and detail commit message next


Thanks
xiezhide