[Qemu-devel] Re: [PATCH 1/2] qemu/qdev: type safety in reset handler

[Michael S. Tsirkin]

On Wed, Sep 16, 2009 at 11:21:41AM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > Add type safety to qdev reset handlers, by declaring them as
> > DeviceState * rather than void *.
> >
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  hw/qdev.c    |   10 ++++++++--
> >  hw/qdev.h    |    3 ++-
> >  hw/rtl8139.c |   10 +++++++---
> >  hw/tcx.c     |   11 +++++++----
> >  4 files changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 43b1beb..4913f88 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -209,6 +209,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >      return qdev;
> >  }
> >  
> > +static void qdev_reset(void *opaque)
> > +{
> > +   DeviceState *dev = opaque;
> > +   dev->info->reset(dev);
> > +}
> > +
> 
> Is there a plan to remove qdev_reset in the future?

When all devices are converted to qdev,
we can convert opaque to DeviceState.

> >  /* Initialize a device.  Device properties should be set before calling
> >     this function.  IRQs and MMIO regions should be connected/mapped after
> >     calling this function.  */
> > @@ -220,7 +226,7 @@ int qdev_init(DeviceState *dev)
> >      if (rc < 0)
> >          return rc;
> >      if (dev->info->reset)
> > -        qemu_register_reset(dev->info->reset, dev);
> > +        qemu_register_reset(qdev_reset, dev);
> 
> Otherwise, I prefer the old version.  We test if there exist
>   dev->info->reset
> and just after that, we use
>   dev->info->reset()

Good point. I think I'll just move the if (dev->info->reset)
into qdev_reset.

> If the plan is to add type checking to the system, they I would like to
> do something like the patch attached (not compiled, just the idea).
> 
> Add a qdev_register_reset() that takes a dev.
> And at reset time, if there is a dev parameter, we call its reset
> function (all with typecheking), and we maintain the func + opaque until
> everyting is ported (no, I don't now if that will ever happens either)
> 
> For users, normal DO_UPCAST() will do the trick on the reset handler.

As you say, we don't know if everything will ever be ported
to your proposed way. Keeping both around seems much worse
than using a void *.

> Later, Juan.
> diff --git a/vl.c b/vl.c
> index eb01da7..facfc67 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3198,6 +3198,7 @@ typedef struct QEMUResetEntry {
>      QTAILQ_ENTRY(QEMUResetEntry) entry;
>      QEMUResetHandler *func;
>      void *opaque;
> +    DeviceState *dev;

So we have both opaque and dev, pointing to the same device.  dev is
used only for reset, and opaque for everything else.  And dev takes
precedence if it is set, opaque is ignored on reset, but if not, opaque
is used. At a minimum, all these rules should be documented?

>  } QEMUResetEntry;
> 
>  static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> @@ -3259,6 +3260,18 @@ void qemu_register_reset(QEMUResetHandler *func, void 
> *opaque)
> 
>      re->func = func;
>      re->opaque = opaque;
> +    re->dev = NULL;
> +    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
> +}
> +
> +void qdev_register_reset(DeviceState *dev)
> +{
> +    QEMUResetEntry *re = qemu_mallocz(sizeof(QEMUResetEntry));
> +
> +    re->func = NULL;
> +    re->opaque = NULL;
> +    re->dev = dev;
> +
>      QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
>  }
> 
> @@ -3281,7 +3294,10 @@ void qemu_system_reset(void)
> 
>      /* reset all devices */
>      QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
> -        re->func(re->opaque);
> +        if (re->dev)
> +            re->dev->info->reset(re->dev);
> +        else
> +            re->func(re->opaque);
>      }
>  }
>