[for-2.9] qom: Make all interface types abstract

"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

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qom/object.c                   |  4 +++
 tests/device-introspect-test.c | 61 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 62 insertions(+), 3 deletions(-)

Am 09.12.2016 um 19:06 schrieb Eduardo Habkost:
> "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
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qom/object.c                   |  4 +++
>  tests/device-introspect-test.c | 61 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 7a05e35..3870c1b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -272,6 +272,10 @@ 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.
> +     */
> +    ti->abstract |= ti->instance_size == 0;

IIRC this is a bool field, so I would prefer to avoid |= by using an
old-fashioned if statement.

The ->abstract = false for interfaces itself makes sense to me, but I'd
appreciate confirmation from Paolo or some other interface expert.

>  
>      ti->class = g_malloc0(ti->class_size);
>  
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 37debc1..d1b0ea6 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);
> @@ -119,5 +172,7 @@ int main(int argc, char **argv)
>      qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
>      qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
>  
> +    qtest_add_func("qom/introspect/abstract-interfaces", test_abstract_interfaces);

I see why you combine the two for reuse, but this path looks odd, it's
supposed to indicate which testsuite the test comes from.
"device/introspect/abstract-qom-interfaces" maybe?

> +
>      return g_test_run();
>  }

Regards,
Andreas

On Mon, Dec 12, 2016 at 03:04:25PM +0100, Andreas Färber wrote:
> Am 09.12.2016 um 19:06 schrieb Eduardo Habkost:
> > "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
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qom/object.c                   |  4 +++
> >  tests/device-introspect-test.c | 61 +++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 62 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qom/object.c b/qom/object.c
> > index 7a05e35..3870c1b 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -272,6 +272,10 @@ 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.
> > +     */
> > +    ti->abstract |= ti->instance_size == 0;
> 
> IIRC this is a bool field, so I would prefer to avoid |= by using an
> old-fashioned if statement.

I was going to use an if statement, but then I tried to make it
simpler using "|=". I will add it back in v2.

> 
> The ->abstract = false for interfaces itself makes sense to me, but I'd
> appreciate confirmation from Paolo or some other interface expert.

Thanks!

[...]
> >  int main(int argc, char **argv)
> >  {
> >      g_test_init(&argc, &argv, NULL);
> > @@ -119,5 +172,7 @@ int main(int argc, char **argv)
> >      qtest_add_func("device/introspect/abstract", test_device_intro_abstract);
> >      qtest_add_func("device/introspect/concrete", test_device_intro_concrete);
> >  
> > +    qtest_add_func("qom/introspect/abstract-interfaces", test_abstract_interfaces);
> 
> I see why you combine the two for reuse, but this path looks odd, it's
> supposed to indicate which testsuite the test comes from.
> "device/introspect/abstract-qom-interfaces" maybe?

I was unsure if I should keep the existing path prefix, or change
to a new one to indicate we are not testing anything specific
about devices. If you prefer to keep using "device/introspect", I
will change it in v2.

> 
> > +
> >      return g_test_run();
> >  }

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Dec 12, 2016 at 03:04:25PM +0100, Andreas Färber wrote:
>> Am 09.12.2016 um 19:06 schrieb Eduardo Habkost:
>> > "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
>> > 
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > ---
>> >  qom/object.c                   |  4 +++
>> >  tests/device-introspect-test.c | 61 +++++++++++++++++++++++++++++++++++++++---
>> >  2 files changed, 62 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/qom/object.c b/qom/object.c
>> > index 7a05e35..3870c1b 100644
>> > --- a/qom/object.c
>> > +++ b/qom/object.c
>> > @@ -272,6 +272,10 @@ 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.
>> > +     */
>> > +    ti->abstract |= ti->instance_size == 0;
>> 
>> IIRC this is a bool field, so I would prefer to avoid |= by using an
>> old-fashioned if statement.
>
> I was going to use an if statement, but then I tried to make it
> simpler using "|=". I will add it back in v2.

Operator | is perfectly fine for boolean operands, and so is your |=
line.  Doesn't mean a conditional would be worse.

[...]