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.
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.
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 37debc1..c5637cc 100644
> --- a/tests/device-introspect-test.c
> +++ b/tests/device-introspect-test.c
> @@ -20,18 +20,24 @@
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qdict.h"
> #include "libqtest.h"
>
> const char common_args[] = "-nodefaults -machine none";
>
> -static QList *device_type_list(bool abstract)
> +static QList *qom_list_types(const char *implements, bool abstract)
> {
> QDict *resp;
> QList *ret;
> + QDict *args = qdict_new();
>
> + qdict_put(args, "abstract", qbool_from_bool(abstract));
> + if (implements) {
> + qdict_put(args, "implements", qstring_from_str(implements));
> + }
> resp = qmp("{'execute': 'qom-list-types',"
> - " 'arguments': {'implements': 'device', 'abstract': %i}}",
> - abstract);
> + " 'arguments': %p }", args);
> g_assert(qdict_haskey(resp, "return"));
> ret = qdict_get_qlist(resp, "return");
> QINCREF(ret);
> @@ -39,6 +45,11 @@ static QList *device_type_list(bool abstract)
> return ret;
> }
>
> +static QList *device_type_list(bool abstract)
> +{
> + return qom_list_types("device", abstract);
> +}
> +
> static void test_one_device(const char *type)
> {
> QDict *resp;
> @@ -110,6 +121,48 @@ static void test_device_intro_concrete(void)
> qtest_end();
> }
>
> +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?
> + g_assert_cmpstr(aname, ==, found);
I'm having a mental block... what exactly is this loop nest testing?
> + }
> +
> + QDECREF(all_types);
> + QDECREF(obj_types);
> + qtest_end();
> +}
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> @@ -118,6 +171,7 @@ int main(int argc, char **argv)
> qtest_add_func("device/introspect/none", test_device_intro_none);
> qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
> qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
> + qtest_add_func("device/introspect/abstract-interfaces",
> test_abstract_interfaces);
>
> return g_test_run();
> }
