[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.9] qom: Make all interface types abstract
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH for-2.9] qom: Make all interface types abstract |
Date: |
Mon, 12 Dec 2016 16:22:56 -0200 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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