[1.3,4/5] qdev: simplify (de)allocation of buses

All conditional deallocation can now be done with object_delete.
Remove the @qom_allocated and @glib_allocated fields; replace the latter
with a direct assignment of the @free function pointer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c       |  2 +-
 hw/qdev-core.h |  5 -----
 hw/qdev.c      | 10 +---------
 hw/sysbus.c    |  2 +-
 4 files changed, 3 insertions(+), 16 deletions(-)

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> All conditional deallocation can now be done with object_delete.
> Remove the @qom_allocated and @glib_allocated fields; replace the latter
> with a direct assignment of the @free function pointer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I am grateful for your suggestion to clean up this mess! However...

> ---
>  hw/pci.c       |  2 +-
>  hw/qdev-core.h |  5 -----
>  hw/qdev.c      | 10 +---------
>  hw/sysbus.c    |  2 +-
>  4 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 9841e39..97a0cd7 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -301,9 +301,9 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>      PCIBus *bus;
>  
>      bus = g_malloc0(sizeof(*bus));
> -    bus->qbus.glib_allocated = true;
>      pci_bus_new_inplace(bus, parent, name, address_space_mem,
>                          address_space_io, devfn_min);
> +    OBJECT(bus)->free = g_free;

Anthony asked not to use the macros like this, but in this case to have
Object *obj; to assign it after g_malloc0() and use obj->free = g_free;
here.

>      return bus;
>  }
>  
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index fce9e22..fff7f0f 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -106,17 +106,12 @@ typedef struct BusChild {
>  
>  /**
>   * BusState:
> - * @qom_allocated: Indicates whether the object was allocated by QOM.
> - * @glib_allocated: Indicates whether the object was initialized in-place
> - * yet is expected to be freed with g_free().
>   */
>  struct BusState {
>      Object obj;
>      DeviceState *parent;
>      const char *name;
>      int allow_hotplug;
> -    bool qom_allocated;
> -    bool glib_allocated;
>      int max_index;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> diff --git a/hw/qdev.c b/hw/qdev.c
> index f43717b..788b4da 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -454,7 +454,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      BusState *bus;
>  
>      bus = BUS(object_new(typename));
> -    bus->qom_allocated = true;
>  
>      bus->parent = parent;
>      bus->name = name ? g_strdup(name) : NULL;
> @@ -465,14 +464,7 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>  
>  void qbus_free(BusState *bus)
>  {
> -    if (bus->qom_allocated) {
> -        object_delete(OBJECT(bus));
> -    } else {
> -        object_finalize(OBJECT(bus));
> -        if (bus->glib_allocated) {
> -            g_free(bus);
> -        }
> -    }
> +    object_delete(OBJECT(bus));

In this case ("free") I have no objection to using "delete" for
simplification. ;)

>  }
>  
>  static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
> diff --git a/hw/sysbus.c b/hw/sysbus.c
> index 4969f06..ef8ffb6 100644
> --- a/hw/sysbus.c
> +++ b/hw/sysbus.c
> @@ -274,7 +274,7 @@ static void main_system_bus_create(void)
>      main_system_bus = g_malloc0(system_bus_info.instance_size);
>      qbus_create_inplace(main_system_bus, TYPE_SYSTEM_BUS, NULL,
>                          "main-system-bus");
> -    main_system_bus->glib_allocated = true;
> +    OBJECT(main_system_bus)->free = g_free;

Same here.

>      object_property_add_child(container_get(qdev_get_machine(),
>                                              "/unattached"),
>                                "sysbus", OBJECT(main_system_bus), NULL);
> 

Andreas

Il 23/11/2012 18:07, Andreas Färber ha scritto:
>> >      bus = g_malloc0(sizeof(*bus));
>> > -    bus->qbus.glib_allocated = true;
>> >      pci_bus_new_inplace(bus, parent, name, address_space_mem,
>> >                          address_space_io, devfn_min);
>> > +    OBJECT(bus)->free = g_free;
> Anthony asked not to use the macros like this, but in this case to have
> Object *obj; to assign it after g_malloc0() and use obj->free = g_free;
> here.

All of the alternatives look ugly:

    /* malloc mismatch, ouch */
    obj = g_malloc0(sizeof(PCIBus));
    bus = PCI_BUS(obj);

    /* Cast to superclass? */
    bus = g_malloc0(sizeof(*bus));
    obj = OBJECT(bus);

    /* Huge path to access the member */
    bus->qbus.obj->free = g_free;

The right solution would be to have qbus_init:

    void qbus_init(BusState *bus,
                   DeviceState *parent, const char *name)
    {
        object_initialize(bus, typename);

        bus->parent = parent;
        bus->name = name ? g_strdup(name) : NULL;
        qbus_realize(bus);
    }

    void qbus_create_inplace(BusState *bus, const char *typename,
                             DeviceState *parent, const char *name)
    {
        object_initialize(bus, typename);
        qbus_init(bus, parent, name);
    }

and just use object_new + qbus_init.  But it is more invasive, I'd
prefer the least pleasant but smaller alternative now.

Paolo