On Wed, Dec 14, 2016 at 02:04:50PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Use old-fashioned if statement instead of "|=" on bool field
> > Suggested-by: Andreas Färber <afaer...@suse.de>
> > * Keep "device/introspect" path prefix on unit test
> > * Suggested-by: Andreas Färber <afaer...@suse.de>
> > ---
> > 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
