Re: [Qemu-devel] [PATCH for-2.9] qom: Make all interface types abstract

[Eduardo Habkost]

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 <address@hidden>
> > ---
> >  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