On Tue, Jul 25, 2017 at 07:21:45PM +0300, Manos Pitsidianakis wrote:
> On Tue, Jul 25, 2017 at 05:09:41PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Jul 25, 2017 at 01:29:08PM +0300, Manos Pitsidianakis wrote:
> > > On Mon, Jul 24, 2017 at 04:12:47PM +0100, Stefan Hajnoczi wrote:
> > > > On Fri, Jul 14, 2017 at 12:45:18PM +0300, Manos Pitsidianakis wrote:
> > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > throttle block filter drive easy creation and configuration of
> > > > > throttle
> > > > > groups in QMP and cli.
> > > > >
> > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > struct for all throttle configuration needs in QMP.
> > > > >
> > > > > ThrottleGroups can be created via CLI as
> > > > > -object throttling-group,id=foo,x-iops-total=100,x-..
> > > >
> > > > Please make the QOM name and struct name consistent. Either
> > > > ThrottleGroup/throttle-group or ThrottlingGroup/throttling-group but not
> > > > ThrottleGroup/throttling-group.
> > >
> > > I did this on purpose because current throttling has ThrottleGroup
> > > internally and throttling.group in the command line. Should we keep this
> > > only in legacy and make it throttle-group everywhere?
> >
> > I don't see a compatibility issue because -drive throttling.group= is a
> > -drive property while THROTTLE_GROUP ("throttling-group") is a QOM class
> > name. They are unrelated and the QOM convention is for the
> > typedef/struct name (ThrottleGroup) to be consistent with the QOM class
> > name.
> >
> > Therefore it should be safe to use "throttle-group" as the QOM class
> > name instead of "throttling-group".
>
> I meant for consistency not compatibility. Otherwise it probably would be
> better to keep throttle-group/ThrottleGroup in the new interfaces.
You could call it ThrottlingGroup ("throttling-group") for consistency.
> >
> > > > > + visit_type_int64(v, name, &value, &local_err);
> > > > > + if (local_err) {
> > > > > + goto fail;
> > > > > + }
> > > > > + if (value < 0) {
> > > > > + error_setg(&local_err, "Property value must be in range "
> > > > > + "[0, %"PRId64"]", INT64_MAX);
> > > >
> > > > Please print the maximum value relevant to the actual field type instead
> > > > of INT64_MAX.
> > >
> > > This checks the limits of the int64 field you give to QOM. I think you
> > > mean
> > > in the value assignment to each field that follows? In any case, since
> > > unsigned is the only smaller field we could convert it to
> > > uint64_t/uint32_t
> > > internally.
> >
> > I'm saying that UNSIGNED fields are silently truncated if the value is
> > larger than UINT_MAX, and also that the error message is misleading
> > since UNSIGNED fields cannot take values in the whole range we print.
>
> Yes, wouldn't it be better to convert the unsigned field burst_length to
> uint64_t and take care of the overflow case? The field describes seconds,
> but there's no reason to keep it that small.
A burst_length of UINT_MAX seconds is over 136 years. I guess unsigned
int was chosen because larger values will never be used :).
That said, making burst_length uint64_t is fine from a compatibility
point of view.
signature.asc
Description: PGP signature
