Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 04.11.2021 um 15:49 hat Paolo Bonzini geschrieben:
> On 11/4/21 15:26, Kevin Wolf wrote:
> > It took me a while to figure out how to deal with this, but I'm quite
> > happy with the result.
> 
> I like it too.
> 
> > > > Oh, and I also wanted to say something about why not just directly using
> > > > the class name, which was my first idea: 'foo': 'iothread' looks more
> > > > like referencing an existing iothread rather than the configuration for
> > > > a new one. I wanted to leave us the option that we could possibly later
> > > > take a string for such options (a QOM path) and then pass the referenced
> > > > object to QMP commands as the proper QOM type.
> > > 
> > > I agree that 'iothread' is going to be confusing when you're referring to
> > > the configuration.
> > > 
> > > Anyway I'm totally fine with 'qom-config:classname'.  Given this
> > > explanation, however, one alternative that makes sense could be
> > > 'classname:full-config'. Then you could use 'classname:config' for the
> > > autoboxed configs---and reserve 'classname' to mean the pointer to an
> > > object.  Classes are (generally) lowercase and QAPI structs are
> > > CamelCase, so there is not much potential for collisions.
> > 
> > Makes sense to me, too.
> > 
> > I just checked and I actually already forbid class names with colons in
> > them (check_name_str() takes care of this), so yes, suffixes actually
> > work on the QAPI level.
> > 
> > If we actually want to use these types in manually written C code, we
> > might have to convert the name to CamelCase, though, for consistency
> > with the coding style.
> > 
> > We already have a function camel_to_upper(), we'd need a new
> > lower_to_camel(), so that from a class 'rng-random', you would get types
> > 'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
> > parent options).
> 
> That's nice.  IMO with these changes the autoboxing becomes again more
> appealing.  With the auto-generated local config struct,
> 
>     { 'class': 'rng-egd',
>       'parent': 'rng-backend',
>       'config': { 'chardev': 'str' } }
> 
> now maps to
> 
>     bool qom_rng_egd_config(Object *obj, RngEgdConfig *config,
>                             Error **errp)
>     {
>         RngEgd *s = RNG_EGD(obj);
> 
>         s->chr_name = g_strdup(config->chardev);
>         return true;
>     }
> 
> The three arguments follow the same prototype as .instance_config:
> 
>     bool (*instance_config)(Object *obj, Visitor *v, Error **errp);
> 
> just with the visitor replaced by a nice C struct.  You started (obviously)
> with the simplest cases, and it's good to check whether easy things remain
> easy, but it seems to me that all but the simplest objects would end up
> using boxed config anyway.

I think I'd still like to have the option of unboxed arguments for the
simpler objects (most user creatable objects are relatively simple), but
flipping the default would make sense if we just automatically create a
named type.

But before I implement anything like this, I'd first like to hear
Markus's opinion because I would prefer to avoid -EMAGIC during review.

> Also (and this is something Markus and I have discussed in the past, but I'm
> not sure if we have actually reached an agreement), I would make
> instance_config return void.  The usual convention *is* to return bool from
> functions that have an Error** and no other return value; however, that's
> because in general there will be more calls to the function than
> definitions.
> 
> In this case, there will be just one call to the ti->instance_config
> function pointer, in object_configure, and N definitions of the function, so
> the ratio and the rationale are reversed.  See object_property_get for an
> example in qom/object.c.

Good point. Though not necessarily one for ti->instance_config because
that's the automatically generated marshaller and returning a bool from
there isn't a problem at all. The function that should definitely return
void is the idiomatic C one that is manually written and called by the
marshaller.

Kevin