| Message ID | 1481567461-2341-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eduardo Habkost <ehabkost@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 <ehabkost@redhat.com> > --- > Changes v1 -> v2: > * Use old-fashioned if statement instead of "|=" on bool field > Suggested-by: Andreas Färber <afaerber@suse.de> > * Keep "device/introspect" path prefix on unit test > * Suggested-by: Andreas Färber <afaerber@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(); > }
On Wed, Dec 14, 2016 at 02:04:50PM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@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 <ehabkost@redhat.com> > > --- > > Changes v1 -> v2: > > * Use old-fashioned if statement instead of "|=" on bool field > > Suggested-by: Andreas Färber <afaerber@suse.de> > > * Keep "device/introspect" path prefix on unit test > > * Suggested-by: Andreas Färber <afaerber@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.
On 14/12/2016 14:48, Eduardo Habkost wrote: >> 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. I think it's worse. Interfaces are abstract by definition. Requiring ".abstract = true" makes things less intuitive. v2 seems good. Paolo
On Wed, Dec 14, 2016 at 06:07:48PM +0100, Paolo Bonzini wrote: > > > On 14/12/2016 14:48, Eduardo Habkost wrote: > >> 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. > > I think it's worse. > > Interfaces are abstract by definition. Requiring ".abstract = true" > makes things less intuitive. v2 seems good. I don't think that making the type declaration more explicit would cause any real problems, and it would help people grepping the code. Anybody forgetting to add .abstract=true to new interfaces would immediatly see an assertion error and would just be annoyed for 1 minute. I will submit v3 and let the maintainers decide.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 14/12/2016 14:48, Eduardo Habkost wrote: >>> 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. > > I think it's worse. > > Interfaces are abstract by definition. Requiring ".abstract = true" > makes things less intuitive. v2 seems good. What makes a TypeInfo declaration an interface? Whatever it is, it better be *locally* obvious.
On 14/12/2016 18:47, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 14/12/2016 14:48, Eduardo Habkost wrote: >>>> 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. >> >> I think it's worse. >> >> Interfaces are abstract by definition. Requiring ".abstract = true" >> makes things less intuitive. v2 seems good. > > What makes a TypeInfo declaration an interface? Whatever it is, it > better be *locally* obvious. The fact that the superclass is an interface: 1) most interface names are (or should be) "interfacey". Compare device, memory backend, console (all classes) with user-creatable, fw path provider, hotplug handler. A few others simply end with "_IF". Of course naming is the hardest problem in computer science so there are some interfaces whose name might apply just as well to a class (stream slave, ISA DMA). However... 2) ... currently we don't have a single case of an interface that doesn't inherit from TYPE_INTERFACE, so all interfaces are declared with ".parent = TYPE_INTERFACE". That does make a TypeInfo obviously an interface. If we ever have a case of interface inheritance, the supertype had better have a good name. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 14/12/2016 18:47, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 14/12/2016 14:48, Eduardo Habkost wrote: >>>>> 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. >>> >>> I think it's worse. >>> >>> Interfaces are abstract by definition. Requiring ".abstract = true" >>> makes things less intuitive. v2 seems good. >> >> What makes a TypeInfo declaration an interface? Whatever it is, it >> better be *locally* obvious. > > The fact that the superclass is an interface: > > 1) most interface names are (or should be) "interfacey". Compare > device, memory backend, console (all classes) with user-creatable, fw > path provider, hotplug handler. A few others simply end with "_IF". Of > course naming is the hardest problem in computer science so there are > some interfaces whose name might apply just as well to a class (stream > slave, ISA DMA). However... > > 2) ... currently we don't have a single case of an interface that > doesn't inherit from TYPE_INTERFACE, so all interfaces are declared with > ".parent = TYPE_INTERFACE". That does make a TypeInfo obviously an > interface. > > If we ever have a case of interface inheritance, the supertype had > better have a good name. I see. The choice is between a complex default for .abstract that permits us to elide .abstract = true for interface types, and a simple default that requires us to spell it out explicitly. Given that we have the grand total of thirteen interface types, I prefer simple & explicit. Thirteen obvious .abstract = true are less of a mental burden than a complex default. Even 25 would be for me.
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); 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) */ + g_assert_cmpstr(aname, ==, found); + } + + 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(); }
"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 <ehabkost@redhat.com> --- Changes v1 -> v2: * Use old-fashioned if statement instead of "|=" on bool field Suggested-by: Andreas Färber <afaerber@suse.de> * Keep "device/introspect" path prefix on unit test * Suggested-by: Andreas Färber <afaerber@suse.de> --- qom/object.c | 6 +++++ tests/device-introspect-test.c | 60 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 63 insertions(+), 3 deletions(-)