* [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
@ 2016-12-12 18:31 Eduardo Habkost
2016-12-14 13:04 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Eduardo Habkost @ 2016-12-12 18:31 UTC (permalink / raw)
To: Andreas Färber; +Cc: Markus Armbruster, Paolo Bonzini, Lin Ma, qemu-devel
"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);
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();
}
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-12 18:31 [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract Eduardo Habkost
@ 2016-12-14 13:04 ` Markus Armbruster
2016-12-14 13:48 ` Eduardo Habkost
0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2016-12-14 13:04 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Andreas Färber, qemu-devel, Paolo Bonzini, Lin Ma
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();
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-14 13:04 ` Markus Armbruster
@ 2016-12-14 13:48 ` Eduardo Habkost
2016-12-14 17:07 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Eduardo Habkost @ 2016-12-14 13:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Andreas Färber, qemu-devel, Paolo Bonzini, Lin Ma
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.
--
Eduardo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-14 13:48 ` Eduardo Habkost
@ 2016-12-14 17:07 ` Paolo Bonzini
2016-12-14 17:45 ` Eduardo Habkost
2016-12-14 17:47 ` Markus Armbruster
0 siblings, 2 replies; 8+ messages in thread
From: Paolo Bonzini @ 2016-12-14 17:07 UTC (permalink / raw)
To: Eduardo Habkost, Markus Armbruster
Cc: Andreas Färber, qemu-devel, Lin Ma
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-14 17:07 ` Paolo Bonzini
@ 2016-12-14 17:45 ` Eduardo Habkost
2016-12-14 17:47 ` Markus Armbruster
1 sibling, 0 replies; 8+ messages in thread
From: Eduardo Habkost @ 2016-12-14 17:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Markus Armbruster, Andreas Färber, qemu-devel, Lin Ma
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.
--
Eduardo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-14 17:07 ` Paolo Bonzini
2016-12-14 17:45 ` Eduardo Habkost
@ 2016-12-14 17:47 ` Markus Armbruster
2016-12-15 13:45 ` Paolo Bonzini
1 sibling, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2016-12-14 17:47 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Eduardo Habkost, Lin Ma, Andreas Färber, qemu-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-14 17:47 ` Markus Armbruster
@ 2016-12-15 13:45 ` Paolo Bonzini
2016-12-19 10:21 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2016-12-15 13:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Eduardo Habkost, Andreas Färber, Lin Ma
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract
2016-12-15 13:45 ` Paolo Bonzini
@ 2016-12-19 10:21 ` Markus Armbruster
0 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2016-12-19 10:21 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Lin Ma, qemu-devel, Andreas Färber, Eduardo Habkost
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-19 10:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-12 18:31 [Qemu-devel] [PATCH for-2.9 v2] qom: Make all interface types abstract Eduardo Habkost
2016-12-14 13:04 ` Markus Armbruster
2016-12-14 13:48 ` Eduardo Habkost
2016-12-14 17:07 ` Paolo Bonzini
2016-12-14 17:45 ` Eduardo Habkost
2016-12-14 17:47 ` Markus Armbruster
2016-12-15 13:45 ` Paolo Bonzini
2016-12-19 10:21 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.