[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Manos Pitsidianakis
Subject: Re: [Qemu-block] [PATCH 4/7] block: convert ThrottleGroup to object with QOM
Date: Tue, 25 Jul 2017 19:21:45 +0300
User-agent: NeoMutt/20170609-57-1e93be (1.8.3)

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

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

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: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]