Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract

[Eduardo Habkost]

On Wed, Dec 14, 2016 at 02:04:50PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
> 
> > "qom-list-types abstract=false" currently returns all interface
> > types, as if they were not abstract. Fix this by making sure all
> > interface types are abstract.
> >
> > All interface types have instance_size == 0, so we can use
> > it to set abstract=true on type_initialize().
> >
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> > * Use old-fashioned if statement instead of "|=" on bool field
> >   Suggested-by: Andreas Färber <address@hidden>
> > * Keep "device/introspect" path prefix on unit test
> >   * Suggested-by: Andreas Färber <address@hidden>
> > ---
> >  qom/object.c                   |  6 +++++
> >  tests/device-introspect-test.c | 60 
> > +++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 7a05e35..760fafb 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -272,6 +272,12 @@ static void type_initialize(TypeImpl *ti)
> >  
> >      ti->class_size = type_class_get_size(ti);
> >      ti->instance_size = type_object_get_size(ti);
> > +    /* Any type with zero instance_size is implicitly abstract.
> > +     * This means interface types are all abstract.
> > +     */
> > +    if (ti->instance_size == 0) {
> > +        ti->abstract = true;
> > +    }
> >  
> >      ti->class = g_malloc0(ti->class_size);
> >  
> 
> Letting zero instance_size imply abstract works (I guess), but is it
> wise to imply?  Is requiring explicit .abstract = true really too much
> trouble?
> 
> Consider:
> 
>     static const TypeInfo uc_interface_info = {
>         .name          = TYPE_USER_CREATABLE,
>         .parent        = TYPE_INTERFACE,
>         .class_size = sizeof(UserCreatableClass),
>     };
> 
> Is this abstract?  Yes, because there's no .instance_size = ...,
> therefore .instance_size remains zero, and .abstract defaults to true.
> 
>     static const TypeInfo memory_region_info = {
>         .parent             = TYPE_OBJECT,
>         .name               = TYPE_MEMORY_REGION,
>         .instance_size      = sizeof(MemoryRegion),
>         .instance_init      = memory_region_initfn,
>         .instance_finalize  = memory_region_finalize,
>     };
> 
> Is this abstract?  No, because with .instance_size =
> sizeof(MemoryRegion), which known to be non-zero, .abstract defaults to
> false.

Let's make it worse:

static const TypeInfo palmetto_bmc_type = {
    .name = MACHINE_TYPE_NAME("palmetto-bmc"),
    .parent = TYPE_MACHINE,
    .class_init = palmetto_bmc_class_init,
};

Is this abstract? No, because instance_size from the parent type is used, and
it is not zero.

> 
> Is such a complex default a good idea?
> 
>    static const TypeInfo rng_backend_info = {
>        .name = TYPE_RNG_BACKEND,
>        .parent = TYPE_OBJECT,
>        .instance_size = sizeof(RngBackend),
>        .instance_init = rng_backend_init,
>        .instance_finalize = rng_backend_finalize,
>        .class_size = sizeof(RngBackendClass),
>        .class_init = rng_backend_class_init,
>        .abstract = true,
>        .interfaces = (InterfaceInfo[]) {
>            { TYPE_USER_CREATABLE },
>            { }
>        }
>    };
> 
> Is this abstract?  Yes, because with .abstract = true, the default
> doesn't matter.
> 
> How do you find all abstract TypeInfo in the source?  The uninitiated
> might grep for .abstract = true, and be misled.  The initiated will be
> annoyed instead, because grepping for *absence* of .instance_size = is
> bothersome.
> 
> I suspect life could be easier going forward if we instead required
> .abstract = true for interfaces, and enforced it with
> assert(ti->instance_size || ti->abstract) here.

I was doing that before deciding to change type_initialize(). I
think I still have the commit in my git reflog, I will recover it
and submit it as v3.

[...]
> > +static void test_abstract_interfaces(void)
> > +{
> > +    QList *all_types;
> > +    QList *obj_types;
> > +    QListEntry *ae;
> > +
> > +    qtest_start(common_args);
> > +    /* qom-list-types implements=interface would return any type
> > +     * that implements _any_ interface (not just interface types),
> > +     * so use a trick to find the interface type names:
> > +     * - list all object types
> > +     * - list all types, and look for items that are not
> > +     *   on the first list
> > +     */
> > +    all_types = qom_list_types(NULL, false);
> > +    obj_types = qom_list_types("object", false);
> > +
> > +    QLIST_FOREACH_ENTRY(all_types, ae) {
> > +        QDict *at = qobject_to_qdict(qlist_entry_obj(ae));
> > +        const char *aname = qdict_get_str(at, "name");
> > +        QListEntry *oe;
> > +        const char *found = NULL;
> > +
> > +        QLIST_FOREACH_ENTRY(obj_types, oe) {
> > +            QDict *ot = qobject_to_qdict(qlist_entry_obj(oe));
> > +            const char *oname = qdict_get_str(ot, "name");
> > +            if (!strcmp(aname, oname)) {
> > +                found = oname;
> > +                break;
> > +            }
> > +        }
> > +
> > +        /* Using g_assert_cmpstr() will give more useful failure
> > +         * messages than g_assert(found) */
> 
> Sure this comment is worth having?

All we need to check here is if 'found' is not NULL. I think I
would be confused by the usage of g_assert_cmpstr() if I was
reading the code, so I decided to add the comment.

> 
> > +        g_assert_cmpstr(aname, ==, found);
> 
> I'm having a mental block...  what exactly is this loop nest testing?

It's implementing the trick described above:
1) list all object types
2) list all types, and look for items that are not on the first
   list.

In other words, checking if all items in all_types are present in
obj_types.

My first attempt was to just check if
  qom-list-types implements="interface" abstract=false
returned an empty list, but it failed because it returns all
object types that implement any interface.

-- 
Eduardo