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

[xiezhide]

> Subject: Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the
> ThrottleLimits member names
> 
> 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.
> 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).
> 

@Erick, thanks for your simple but exact explaining for purpose of this patch

Thanks
xiezhide


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