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

[Eduardo Habkost]

On Thu, Dec 15, 2016 at 10:17:27AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > ---
> > Changes v1 -> v2:
> > * Use old-fashioned if statement instead of "|=" on bool field
> >   Suggested-by: Andreas Färber <address@hidden>
> > * Keep "device/introspect" path prefix on unit test
> >   * Suggested-by: Andreas Färber <address@hidden>
> >
> > Changes v2 -> v3:
> > * Instead of making interfaces implicitly abstract,
> >   set abstract=true explicitly for all of them and replace
> >   type_initialize() abstract=true code with an assert().
> >   Suggested-by: Markus Armbruster <address@hidden>
> > * Try to make test code a bit more clear
> > ---
> >  hw/acpi/acpi_interface.c       |  1 +
> >  hw/arm/boot.c                  |  1 +
> >  hw/core/fw-path-provider.c     |  1 +
> >  hw/core/hotplug.c              |  1 +
> >  hw/core/nmi.c                  |  1 +
> >  hw/core/stream.c               |  1 +
> >  hw/intc/intc.c                 |  1 +
> >  hw/ipmi/ipmi.c                 |  1 +
> >  hw/isa/isa-bus.c               |  1 +
> >  hw/ppc/pnv_xscom.c             |  1 +
> >  hw/timer/m48t59.c              |  1 +
> >  qom/object.c                   |  2 ++
> >  qom/object_interfaces.c        |  1 +
> >  tests/check-qom-interface.c    |  1 +
> >  tests/device-introspect-test.c | 58 
> > +++++++++++++++++++++++++++++++++++++++---
> >  15 files changed, 70 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c
> > index 6583917..49388a9 100644
> > --- a/hw/acpi/acpi_interface.c
> > +++ b/hw/acpi/acpi_interface.c
> > @@ -16,6 +16,7 @@ static void register_types(void)
> >      static const TypeInfo acpi_dev_if_info = {
> >          .name          = TYPE_ACPI_DEVICE_IF,
> >          .parent        = TYPE_INTERFACE,
> > +        .abstract = true,
> >          .class_size = sizeof(AcpiDeviceIfClass),
> >      };
> >  
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index ff621e4..65c59b5 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -1031,6 +1031,7 @@ void arm_load_kernel(ARMCPU *cpu, struct 
> > arm_boot_info *info)
> >  static const TypeInfo arm_linux_boot_if_info = {
> >      .name = TYPE_ARM_LINUX_BOOT_IF,
> >      .parent = TYPE_INTERFACE,
> > +    .abstract = true,
> >      .class_size = sizeof(ARMLinuxBootIfClass),
> >  };
> >  
> > diff --git a/hw/core/fw-path-provider.c b/hw/core/fw-path-provider.c
> > index 33b9983..96f3131 100644
> > --- a/hw/core/fw-path-provider.c
> > +++ b/hw/core/fw-path-provider.c
> > @@ -42,6 +42,7 @@ char *fw_path_provider_try_get_dev_path(Object *o, 
> > BusState *bus,
> >  static const TypeInfo fw_path_provider_info = {
> >      .name          = TYPE_FW_PATH_PROVIDER,
> >      .parent        = TYPE_INTERFACE,
> > +    .abstract      = true,
> >      .class_size    = sizeof(FWPathProviderClass),
> >  };
> >  
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > index 17ac986..f74fba5 100644
> > --- a/hw/core/hotplug.c
> > +++ b/hw/core/hotplug.c
> > @@ -60,6 +60,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
> >  static const TypeInfo hotplug_handler_info = {
> >      .name          = TYPE_HOTPLUG_HANDLER,
> >      .parent        = TYPE_INTERFACE,
> > +    .abstract      = true,
> >      .class_size = sizeof(HotplugHandlerClass),
> >  };
> >  
> > diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> > index bfd0896..403edd3 100644
> > --- a/hw/core/nmi.c
> > +++ b/hw/core/nmi.c
> > @@ -76,6 +76,7 @@ void nmi_monitor_handle(int cpu_index, Error **errp)
> >  static const TypeInfo nmi_info = {
> >      .name          = TYPE_NMI,
> >      .parent        = TYPE_INTERFACE,
> > +    .abstract      = true,
> >      .class_size    = sizeof(NMIClass),
> >  };
> >  
> > diff --git a/hw/core/stream.c b/hw/core/stream.c
> > index 4439ecd..db7f90b 100644
> > --- a/hw/core/stream.c
> > +++ b/hw/core/stream.c
> > @@ -21,6 +21,7 @@ stream_can_push(StreamSlave *sink, StreamCanPushNotifyFn 
> > notify,
> >  static const TypeInfo stream_slave_info = {
> >      .name          = TYPE_STREAM_SLAVE,
> >      .parent        = TYPE_INTERFACE,
> > +    .abstract      = true,
> >      .class_size = sizeof(StreamSlaveClass),
> >  };
> >  
> > diff --git a/hw/intc/intc.c b/hw/intc/intc.c
> > index 2e1e29e..9373546 100644
> > --- a/hw/intc/intc.c
> > +++ b/hw/intc/intc.c
> > @@ -29,6 +29,7 @@
> >  static const TypeInfo intctrl_info = {
> >      .name = TYPE_INTERRUPT_STATS_PROVIDER,
> >      .parent = TYPE_INTERFACE,
> > +    .abstract = true,
> >      .class_size = sizeof(InterruptStatsProviderClass),
> >  };
> >  
> > diff --git a/hw/ipmi/ipmi.c b/hw/ipmi/ipmi.c
> > index 5cf1caa..c255499 100644
> > --- a/hw/ipmi/ipmi.c
> > +++ b/hw/ipmi/ipmi.c
> > @@ -86,6 +86,7 @@ static void ipmi_interface_class_init(ObjectClass *class, 
> > void *data)
> >  static TypeInfo ipmi_interface_type_info = {
> >      .name = TYPE_IPMI_INTERFACE,
> >      .parent = TYPE_INTERFACE,
> > +    .abstract = true,
> >      .class_size = sizeof(IPMIInterfaceClass),
> >      .class_init = ipmi_interface_class_init,
> >  };
> > diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> > index 9d07b11..a1cad86 100644
> > --- a/hw/isa/isa-bus.c
> > +++ b/hw/isa/isa-bus.c
> > @@ -41,6 +41,7 @@ static void isa_bus_class_init(ObjectClass *klass, void 
> > *data)
> >  static const TypeInfo isa_dma_info = {
> >      .name = TYPE_ISADMA,
> >      .parent = TYPE_INTERFACE,
> > +    .abstract = true,
> >      .class_size = sizeof(IsaDmaClass),
> >  };
> >  
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index 8da2718..642f104 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -192,6 +192,7 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
> >  static const TypeInfo pnv_xscom_interface_info = {
> >      .name = TYPE_PNV_XSCOM_INTERFACE,
> >      .parent = TYPE_INTERFACE,
> > +    .abstract = true,
> >      .class_size = sizeof(PnvXScomInterfaceClass),
> >  };
> >  
> > diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> > index e46ca88..42c5fc7 100644
> > --- a/hw/timer/m48t59.c
> > +++ b/hw/timer/m48t59.c
> > @@ -884,6 +884,7 @@ static void 
> > m48txx_sysbus_concrete_class_init(ObjectClass *klass, void *data)
> >  static const TypeInfo nvram_info = {
> >      .name = TYPE_NVRAM,
> >      .parent = TYPE_INTERFACE,
> > +    .abstract = true,
> >      .class_size = sizeof(NvramClass),
> >  };
> >  
> > diff --git a/qom/object.c b/qom/object.c
> > index 7a05e35..d93989a 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -272,6 +272,8 @@ static void type_initialize(TypeImpl *ti)
> >  
> >      ti->class_size = type_class_get_size(ti);
> >      ti->instance_size = type_object_get_size(ti);
> > +    /* types with zero instance_size can't be instantiated, must be 
> > abstract */
> > +    assert(ti->instance_size > 0 || ti->abstract);
> >  
> >      ti->class = g_malloc0(ti->class_size);
> >  
> 
> Separate idea: should type_initialize_interface() check
> !interface_type->instance_size?
> 
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index ded4d84..f6dc46c 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -217,6 +217,7 @@ static void register_types(void)
> >      static const TypeInfo uc_interface_info = {
> >          .name          = TYPE_USER_CREATABLE,
> >          .parent        = TYPE_INTERFACE,
> > +        .abstract      = true,
> >          .class_size = sizeof(UserCreatableClass),
> >      };
> >  
> > diff --git a/tests/check-qom-interface.c b/tests/check-qom-interface.c
> > index f87c9aa..560b7e2 100644
> > --- a/tests/check-qom-interface.c
> > +++ b/tests/check-qom-interface.c
> > @@ -36,6 +36,7 @@ typedef struct TestIfClass {
> >  static const TypeInfo test_if_info = {
> >      .name          = TYPE_TEST_IF,
> >      .parent        = TYPE_INTERFACE,
> > +    .abstract      = true,
> >      .class_size = sizeof(TestIfClass),
> >  };
> >  
> 
> Just 13 .abstract = true; fairly harmless.
> 
> > diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> > index 37debc1..4fa8d29 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,46 @@ 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:
> > +     * 1) list all non-abstract types
> > +     * 2) list all non-abstract object types
> > +     * 3) Look for items in (1) that are not in (2)
> > +     */
> > +    all_types = qom_list_types(NULL, false);
> 
> All concrete types.
> 
> > +    obj_types = qom_list_types("object", false);
> 
> All concrete subtypes of "object".
> 
> > +
> > +    QLIST_FOREACH_ENTRY(all_types, ae) {
> 
> For @at in concrete types:
> 
> > +        QDict *at = qobject_to_qdict(qlist_entry_obj(ae));
> > +        const char *aname = qdict_get_str(at, "name");
> > +        QListEntry *oe;
> > +        const char *found = NULL;
> > +
> 
> Search the list of concrete subtypes of "object" for @at
> 
> > +        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;
> > +            }
> > +        }
> > +
> > +        g_assert(found);
> 
> Assert @at is a concrete subtype of "object".
> 
> Taken together: assert that any concrete type is a subtype of "object".
> Correct?
> 
> How's that related to interfaces?

The only types that are not subtypes of "object" are interfaces.
If we see any concrete type that is not subtype of "object", it
is a non-abstract interface (or something even weirder).

Anyway, I believe this test case is complete unnecessary in v3.
It made sense to make sure I was adding the automatic
abstract=true line at the right place in the code. Now we have
explicitly changed all interfaces to have abstract=true, and
added an assert() to the code. This test code can be removed from
the patch.

> 
> > +    }
> > +
> > +    QDECREF(all_types);
> > +    QDECREF(obj_types);
> > +    qtest_end();
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      g_test_init(&argc, &argv, NULL);
> > @@ -118,6 +169,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();
> >  }

-- 
Eduardo