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

[Paolo Bonzini]

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.

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.

Thanks,

Paolo