[Top][All Lists]

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

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

From: Paolo Bonzini
Subject: Re: [RFC PATCH 00/12] QOM/QAPI integration part 1
Date: Thu, 4 Nov 2021 15:49:22 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

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.



reply via email to

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