Re: [Qemu-devel] [PATCH v3 2/4] fsdev: QMP interface for throttling

[Pradeep Jagadeesh]

On 5/4/2017 5:19 PM, Eric Blake wrote:
> On 05/04/2017 10:12 AM, Pradeep Jagadeesh wrote:
>
> > > > > > +    IOThrottle throttle = {
> > > > > > +        .has_id = true,
> > > > > > +        .id = (char *) qdict_get_str(qdict, "id"),
> > > > > > +        .bps = qdict_get_int(qdict, "bps"),
> > > > > > +        .bps_rd = qdict_get_int(qdict, "bps_rd"),
> > > > > > +        .bps_wr = qdict_get_int(qdict, "bps_wr"),
> > > > > > +        .iops = qdict_get_int(qdict, "iops"),
> > > > > > +        .iops_rd = qdict_get_int(qdict, "iops_rd"),
> > > > > > +        .iops_wr = qdict_get_int(qdict, "iops_wr"),
> > > > >
> > > > > Again, don't you need to be setting .has_bps=true and so on?
> > > > Same as above. I have not set any max burst values here, because I
> > > > wanted to keep it in line with the block device.
> > > > May be there is a room to enable these max values in both in future.
> > >
> > > I don't think you were getting the point of my question. Since 'bps' is
> > > an optional member of struct IOThrottle, you have to set 'has_bps' at
> > > the same time to make it obvious that 'bps' should affect things.
> > I need some more clarifications here.
> > I did not understand what do you mean by an optional parameter?
> > You mean I need to set "has_*" for all the parameters?
>
> Yes, any optional parameter (one named '*foo' in the .json file) has a
> counterpart has_foo boolean, which should be set to true when the
> parameter is in use (and thus making it obvious when it is set to false
> that it is not in use).
OK, but here the id is only with * (Optional), others are not. Still 
need them to set?

> > > I understand that you aren't setting 'bps_max', nor it's counterpart
> > > 'has_bps_max', and that's not what I was asking about.
> > >
> > >
> > > > > > +++ b/qapi/iothrottle.json
> > > > > > @@ -3,6 +3,7 @@
> > > > > >  ##
> > > > > >  # == QAPI IOThrottle definitions
> > > > > >  ##
> > > > > > +##
> > > > > >  # @IOThrottle:
> > > > >
> > > > > This looks like a spurious change
> > > > You mean the below sentence is not required?
> > >
> > > No, the above addition of a second ## is not required.
> > If I remove that I will get the compilation error.
> > I think the parser needs that.
>
> If the parser chokes without it, then that implies patch 1 is broken,
> and the fix should be squashed there for full bisectability.  Either
> way, it looks like a spurious change when it is occurring in patch 3.
Yes, I reordered the patches and fixed this issue.

-Pradeep