Re: [PATCH] include/hw/core/qdev.h: Fix where device_class_set_parent_realize docs suggest putting parent_realize

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Oct 15, 2026 at 05:33:52PM +0100, Peter Maydell wrote:
> The documentation for device_class_set_parent_realize() suggests that
> the parent realize function pointer is "usually in the instance
> structure".  This is definitely wrong, because the function pointer
> is a class thing, and should live in a class struct, not an instance
> struct.  Update the documentation to correct this, instead
> recommending that it should be placed in the class struct for the
> child class.
> 
> At time of writing, of the 75 or so uses of this function, about 50
> put the parent_realize pointer in the child class struct, and about
> 25 in the parent class struct.  We recommend the former both because
> it is the more common choice, and also because it makes more logical
> sense: it's the child that knows (a) that it exists at all and that
> (b) it wants to have a realize method that will mean it needs to
> chain back to the parent, and we shouldn't need to edit the parent's
> class struct because we want to add a child class that chains to its
> realize method.

I'm confused why we need to store a copy of the parent's
realize method at all, when we can just reference it from
the parent class struct ?

Taking one example

  static void pl050_kbd_realize(DeviceState *dev, Error **errp)
  {
      PL050DeviceClass *pdc = PL050_GET_CLASS(dev);
      PL050KbdState *s = PL050_KBD_DEVICE(dev);
      PL050State *ps = PL050(dev);

      if (!sysbus_realize(SYS_BUS_DEVICE(&s->kbd), errp)) {
          return;
      }

      ps->ps2dev = PS2_DEVICE(&s->kbd);
      pdc->parent_realize(dev, errp);
  }

why wouldn't we rewrite that as

  static void pl050_kbd_realize(DeviceState *dev, Error **errp)
  {
      PL050DeviceClass *pdc = PL050_GET_CLASS(dev);
      DeviceClass *parent = DEVICE_CLASS(object_class_get_parent(OBJECT_CLASS(pdc)));
      PL050KbdState *s = PL050_KBD_DEVICE(dev);
      PL050State *ps = PL050(dev);

      if (!sysbus_realize(SYS_BUS_DEVICE(&s->kbd), errp)) {
          return;
      }

      ps->ps2dev = PS2_DEVICE(&s->kbd);
      parent->realize(dev, errp);
  }

am I missing a reason we need to store the parent's
realize method pointer ?


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/core/qdev.h | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/core/qdev.h b/include/hw/core/qdev.h
> index 8e91e2fcb08..1330e0153d4 100644
> --- a/include/hw/core/qdev.h
> +++ b/include/hw/core/qdev.h
> @@ -953,9 +953,18 @@ void device_class_set_props_n(DeviceClass *dc, const Property *props, size_t n);
>   *
>   * This is intended to be used when the new realize function will
>   * eventually call its parent realization function during creation.
> - * This requires storing the function call somewhere (usually in the
> - * instance structure) so you can eventually call
> - * dc->parent_realize(dev, errp)
> + * @parent_realize should be where you want to save the pointer to
> + * the parent class realize function; the recommended location is a
> + * field named "parent_realize" in your class's class struct.
> + * (Some existing code puts the field in the parent class's class
> + * struct; this will work but is not the preferred thing for new code.)
> + *
> + * In your realize function you should chain to your parent's
> + * realize function using something like::
> + *
> + *    MyThingClass *mc = MY_THING_GET_CLASS(dev);
> + *    (do your child class realize actions here)
> + *    mc->parent_realize(dev, errp);
>   */
>  void device_class_set_parent_realize(DeviceClass *dc,
>                                       DeviceRealize dev_realize,
> -- 
> 2.43.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|