Re: [Qemu-devel] [RFC] QDev explicit constructors & destructors

[Markus Armbruster]

Copying Andreas.

Liviu Ionescu <address@hidden> writes:

> after some more experimenting, here are the conclusions:
>
> - I added a new .construct pointer to all my classes:
>
>     void (*construct)(Object *obj, void *data);
>
> the 'data' pointer is optional, for qdev calls, parameters should be passed 
> via properties.
>
> - constructors should manually call their parent constructor
>
> static void stm32_mcu_construct_callback(Object *obj, void *data)
> {
>     /* Call parent constructor */
>     CORTEXM_MCU_GET_CLASS(obj)->construct(obj, NULL);
>
>     ...
> }
>
> this is intentional, to allow the caller to set additional properties:
>
> static void stm32_mcus_construct_callback(Object *obj, void *data)
> {
>     STM32DeviceClass *st_class = STM32_DEVICE_GET_CLASS(obj);
>     STM32PartInfo *part_info = st_class->part_info;
>     DeviceState *dev = DEVICE(obj);
>     /*
>      * Set additional constructor parameters, that were passed via
>      * the .class_data and copied to custom class member.
>      */
>     qdev_prop_set_ptr(dev, "param-cortexm-capabilities",
>             (void *) &part_info->cortexm);
>     qdev_prop_set_ptr(dev, "param-stm32-capabilities",
>             (void *) part_info->stm32);
>
>     STM32_MCU_GET_CLASS(obj)->construct(obj, NULL);
>
>     ...
> }
>
> - using this mechanism is pretty simple:
>
>     DeviceState *mcu = qdev_alloc(NULL, TYPE_STM32F103RB);
>     {
>         qdev_prop_set_ptr(mcu, "param-machine", machine);
>         STM32_DEVICE_GET_CLASS(mcu)->construct(OBJECT(mcu), NULL);
>
>         /* Set the board specific oscillator frequencies. */
>         qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
>         qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
>     }
>     qdev_realize(mcu);
>
>
> - once the explicit constructors are in place, the .realize callbacks can be 
> freed of other duties and return to their intended function, to propagate the 
> "realized" event to parents and children:
>
> static void stm32_mcu_realize_callback(DeviceState *dev, Error **errp)
> {
>     /* Call parent realize(). */
>     if (!qdev_parent_realize(dev, errp, TYPE_STM32_MCU)) {
>         return;
>     }
>
>     STM32MCUState *state = STM32_MCU_STATE(dev);
>
>     /* Propagate realize() to children. */
>
>     /* RCC */
>     qdev_realize(DEVICE(state->rcc));
>
>     /* FLASH */
>     qdev_realize(DEVICE(state->flash));
>
>     /* GPIO[A-G] */
>     for (int i = 0; i < STM32_MAX_GPIO; ++i) {
>         /* Realize all initialised GPIOs */
>         if (state->gpio[i]) {
>             qdev_realize(DEVICE(state->gpio[i]));
>         }
>     }
> }
>
>
> - as already mentioned, in my current implementation the .construct member is 
> added to each class, but this is not needed as redundant, and should be added 
> once to ObjectClass or at least to DeviceClass.
>
> this would greatly simplify setting it and calling the constructor:
>
>       object_construct(obj, data);
>
>         qdev_prop_set_ptr(dev, "param-machine", machine);     
>       qdev_construct(dev);
>
> (for qdev the params are passed via properties, the optional data pointer is 
> not used)
>
>
> Peter, could you consider this proposal?
>
> should I prepare a patch for it? 
>
> adding this extra field in the class structure does not break compatibility 
> with existing code, but would help reduce the mess with existing .realize, 
> which is abused to do whatever initialisations were not possible in 
> .instance_init; I expect that this will make future code easier to understand 
> and maintain.
>
>
> regards,
>
> Liviu