Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 03.12.2020 um 17:50 hat Paolo Bonzini geschrieben:
> On 03/12/20 16:15, Kevin Wolf wrote:
> > I don't think this is an intermediate state like Eduardo wants to have.
> > Creating the object, then setting properties, then realize [1] will fail
> > after your change. But keeping it working was the whole point of the
> > exercise.
> 
> With the sample code, you must remove object_class_property_set calls at the
> same time as you remove the setters.  Usually that'd be when you convert to
> QAPI and oc->configure, but it doesn't have to be that way if there are good
> reasons not to do so.

Okay, thanks, I think I understand now.

So I assume that in the common case, we'll never have the state that you
describe, but we'll want to directly skip to QAPI generated code. But
it's good to know that we can make smaller steps if we need to in more
complicated cases.

> Also, it still allows you to do so one class at a time, and I *think* the
> presence of subclasses or superclasses doesn't matter (only whether
> properties are still writable).  We can use chardevs (see ChardevCommon in
> qapi/char.json) to validate that before tackling devices.

Yes, it looks like it should be working.

> (In fact, this means that your series---plus -object and object_add
> conversion---would be good, pretty much unchanged, as a first step.  The
> second would be adding oc->configure and object_configure, and converting
> all user-creatable objects to oc->configure.  The third would involve QAPI
> code generation).

I think I'd want to do step 2 and 3 combined, because converting
user-creatable objects to oc->configure means manually writing the
configure function that will be generated from QAPI in step 3. Writing
code just to throw it away isn't my favourite pastime.

> > I'm also not really sure why you go from RngEgdOptions to QObject to a
> > visitor, only to reconstruct RngEgdOptions at the end.
> 
> The two visits are just because you cannot create an input visitor directly
> on C data. I stole that from your patch 18/18 actually, just with
> object_new+object_configure instead of user_creatable_add_type.
> 
> But I wouldn't read too much in the automatically-generated *_new functions
> since they are already in QAPI code generator territory. Instead the basic
> object_configure idea can be applied even without having automatic code
> generation.

Yes, I was just wondering why we're going through visitors at all. But
this is what provides the compatibility with the old property system, so
it makes sense if you need an intermediate step.

> > I think the class
> > implementations should have a normal C interface without visitors and we
> > should be able to just pass the existing RngEgdOptions object (or the
> > individual values for its fields for 'boxed': false).
> 
> Sure, however that requires changes to the QAPI code generator which was
> only item (3) in your list list.  Until then you can already work with a
> visitor interface:
> 
>   void rng_egd_configure(Object *obj, Visitor *v, Error **errp)
>   {
>       RngEgd *s = RNG_EGD(obj);
>       s->config = g_new0(MemoryBackendOptions, 1);
>       visit_type_MemoryBackendOptions(v, NULL, &s->config, errp);
> 
>       s->config->share = (s->config->has_share
>                           ? s->config->share : false);
>       ...
>   }
> 
> but if you had a QAPI description
> 
>   { 'object': 'RngEgd',
>     'qom-type': 'rng-egd',
>     'configuration': 'RngEgdOptions',
>     'boxed': true
>   }
> 
> the QAPI generator could produce the oc->configure implementation. Similar
> to commands, that implementation would be an unmarshaling wrapper that calls
> out to the natural C interface:
> 
>   void qapi_RngEgd_configure(Object *obj, Visitor *v, Error **errp);
>   {
>       Error *local_err = NULL;
>       g_autoptr(MemoryBackendOptions) *config =
>           g_new0(MemoryBackendOptions, 1);
>       visit_type_MemoryBackendOptions(v, NULL, &s->config, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
>           return;
>       }
>       qom_rng_egd_configure(RNG_EGD(obj), config, errp);
>   }
> 
>   void qom_rng_egd_configure(RngEng *s,
>                              RngEgdOptions *config,
>                              Error **errp)
>   {
>       config->share = (config->has_share
>                        ? config->share : false);
>       ...
>       s->config = QAPI_CLONE(RngEgdOptions, config);
>   }

Yes, exactly.

Kevin