Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]

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.
>
>> > > +    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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]