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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Jun 26, 2017 at 06:24:09PM +0300, Manos Pitsidianakis wrote:
> On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
> > > +static void throttle_group_set(Object *obj, Visitor *v, const char * name,
> > > +        void *opaque, Error **errp)
> > > +
> > > +{
> > > +    ThrottleGroup *tg = THROTTLE_GROUP(obj);
> > > +    ThrottleConfig cfg = tg->ts.cfg;
> > > +    Error *local_err = NULL;
> > > +    ThrottleParamInfo *info = opaque;
> > > +    int64_t value;
> > 
> > What happens if this property is set while QEMU is already running?
> 
> I assume you mean setting a property while a group has active members and
> requests? My best answer would be "don't do that". I couldn't figure a way
> to do this cleanly. Individual limit changes may render a ThrottleConfig
> invalid, so it should not be allowed. ThrottleGroups and throttle nodes
> should be destroyed and recreated to change limits with this version, but in
> the next it will be done through block_set_io_throttle() which is the
> existing way to change limits and check for validity. This was discussed in
> the proposal about the new syntax we had on the list.

Please ask on IRC or the mailing list if you have questions.

If you are aware of a limitation and don't know the answer then
submitting the code without any comment or question is dangerous.
Reviewers may miss the problem :) and broken code gets merged.

UserCreatableClass has a ->complete() callback.  You can use this to
perform final initialization and then "freeze" the properties by setting
a bool flag.  The property accessor functions can do:

  if (tg->init_complete) {
      error_setg(errp, "Property modification is not allowed at run-time");
      return;
  }

Something like this is used by
backends/hostmem.c:host_memory_backend_set_size(), for example.

> > 
> > > +        goto out;
> > > +    }
> > 
> > This doesn't validate inputs properly for non int64_t types.
> > 
> > I'm also worried that the command-line bps=,iops=,... options do not
> > have unsigned or double types.  Input ranges and validation should match
> > the QEMU command-line (I know this is a bit of a pain with QOM since the
> > property types are different from QEMU option types).
> 
> I don't know what should be done about this, to be honest, except for
> manually checking the limits for each datatype in the QOM setters.

I believe all throttling parameter types are int64_t in QemuOpts.  If we
want to be compatible with the command-line parameters then they should
also be int64_t here instead of unsigned int or double.

This approach makes sense from the QMP user perspective.  QMP clients
shouldn't have to deal with different data types depending on which
throttling API they use.  Let's keep it consistent - there's no real
drawback to using int64_t.

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