Re: [Qemu-devel] [PATCH 19/24] qdev: move reset handler list from vl.c to qdev.c

[Eduardo Habkost]

On Thu, Nov 15, 2012 at 02:54:57AM +0100, Andreas Färber wrote:
> Am 09.11.2012 15:56, schrieb Eduardo Habkost:
> > The core qdev code uses the reset handler list from vl.c, so move
> > qemu_register_reset(), qemu_unregister_reset() and qemu_devices_reset()
> > to qdev.c.
> > 
> > The function declarations were moved to a new qdev-reset.h file, that is
> > included by hw.h to keep compatibility, so we don't need to change all
> > files that use qemu_register_reset().
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  hw/hw.h         |  6 +-----
> >  hw/qdev-reset.h | 11 +++++++++++
> >  hw/qdev.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> >  hw/qdev.h       |  1 +
> >  sysemu.h        |  1 -
> >  vl.c            | 40 ----------------------------------------
> >  6 files changed, 54 insertions(+), 46 deletions(-)
> >  create mode 100644 hw/qdev-reset.h
> > 
> > diff --git a/hw/hw.h b/hw/hw.h
> > index f530f6f..622a157 100644
> > --- a/hw/hw.h
> > +++ b/hw/hw.h
> > @@ -14,6 +14,7 @@
> >  #include "qemu-file.h"
> >  #include "vmstate.h"
> >  #include "qemu-log.h"
> > +#include "qdev-reset.h"
> >  
> >  #ifdef NEED_CPU_H
> >  #if TARGET_LONG_BITS == 64
> > @@ -37,11 +38,6 @@
> >  #endif
> >  #endif
> >  
> > -typedef void QEMUResetHandler(void *opaque);
> > -
> > -void qemu_register_reset(QEMUResetHandler *func, void *opaque);
> > -void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
> > -
> >  /* handler to set the boot_device order for a specific type of QEMUMachine 
> > */
> >  /* return 0 if success */
> >  typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);
> > diff --git a/hw/qdev-reset.h b/hw/qdev-reset.h
> > new file mode 100644
> > index 0000000..40ae9a5
> > --- /dev/null
> > +++ b/hw/qdev-reset.h
> > @@ -0,0 +1,11 @@
> > +/* Device reset handler function registration, used by qdev */
> > +#ifndef QDEV_RESET_H
> > +#define QDEV_RESET_H
> > +
> > +typedef void QEMUResetHandler(void *opaque);
> > +
> > +void qemu_register_reset(QEMUResetHandler *func, void *opaque);
> > +void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
> > +void qemu_devices_reset(void);
> > +
> > +#endif /* QDEV_RESET_H */
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 2cc6434..c242097 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -35,6 +35,47 @@ int qdev_hotplug = 0;
> >  static bool qdev_hot_added = false;
> >  static bool qdev_hot_removed = false;
> >  
> > +typedef struct QEMUResetEntry {
> > +    QTAILQ_ENTRY(QEMUResetEntry) entry;
> > +    QEMUResetHandler *func;
> > +    void *opaque;
> > +} QEMUResetEntry;
> > +
> > +static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> > +    QTAILQ_HEAD_INITIALIZER(reset_handlers);
> > +
> > +void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> > +{
> > +    QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
> > +
> > +    re->func = func;
> > +    re->opaque = opaque;
> > +    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
> > +}
> > +
> > +void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
> > +{
> > +    QEMUResetEntry *re;
> > +
> > +    QTAILQ_FOREACH(re, &reset_handlers, entry) {
> > +        if (re->func == func && re->opaque == opaque) {
> > +            QTAILQ_REMOVE(&reset_handlers, re, entry);
> > +            g_free(re);
> > +            return;
> > +        }
> > +    }
> > +}
> 
> My tired mind does not like this move and the naming qdev-reset.h.
> The reset handling infrastructure is not limited to DeviceState (qdev),
> it takes an opaque and is limited to softmmu whereas qdev prefers its
> own DeviceClass::reset hook.

True, it isn't qdev-specific. But it doesn't belong to vl.c either.
Should we create a reset.o file just for those few lines of code, or is
there any obvious place where this code can go?

DeviceState CPUs have to be reset too, and DeviceState uses the
reset-handler system to make sure DeviceState objects are reset, so it
won't be softmmu-specific.

An alternative is to make empty stubs for qemu_[un]register_reset(), and
not move the reset-handler system to *-user. But somehow I feel better
having a working qemu_register_reset() function (and then adding a
qemu_devices_reset() call to *-user) than having a fake
qemu_register_reset() function and having to use a different API to
reset the CPUs on on *-user.

> 
> Andreas
> 
> > +
> > +void qemu_devices_reset(void)
> > +{
> > +    QEMUResetEntry *re, *nre;
> > +
> > +    /* reset all devices */
> > +    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
> > +        re->func(re->opaque);
> > +    }
> > +}
> > +
> >  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
> >  {
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > diff --git a/hw/qdev.h b/hw/qdev.h
> > index 365b8d6..2487b3b 100644
> > --- a/hw/qdev.h
> > +++ b/hw/qdev.h
> > @@ -5,5 +5,6 @@
> >  #include "qdev-core.h"
> >  #include "qdev-properties.h"
> >  #include "qdev-monitor.h"
> > +#include "qdev-reset.h"
> >  
> >  #endif
> > diff --git a/sysemu.h b/sysemu.h
> > index ab1ef8b..51f19cc 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -57,7 +57,6 @@ void qemu_system_vmstop_request(RunState reason);
> >  int qemu_shutdown_requested_get(void);
> >  int qemu_reset_requested_get(void);
> >  void qemu_system_killed(int signal, pid_t pid);
> > -void qemu_devices_reset(void);
> >  void qemu_system_reset(bool report);
> >  
> >  void qemu_add_exit_notifier(Notifier *notify);
> > diff --git a/vl.c b/vl.c
> > index 4f03a72..c7448a2 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1456,14 +1456,6 @@ void vm_start(void)
> >  
> >  /* reset/shutdown handler */
> >  
> > -typedef struct QEMUResetEntry {
> > -    QTAILQ_ENTRY(QEMUResetEntry) entry;
> > -    QEMUResetHandler *func;
> > -    void *opaque;
> > -} QEMUResetEntry;
> > -
> > -static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> > -    QTAILQ_HEAD_INITIALIZER(reset_handlers);
> >  static int reset_requested;
> >  static int shutdown_requested, shutdown_signal = -1;
> >  static pid_t shutdown_pid;
> > @@ -1560,38 +1552,6 @@ static bool qemu_vmstop_requested(RunState *r)
> >      return false;
> >  }
> >  
> > -void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> > -{
> > -    QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
> > -
> > -    re->func = func;
> > -    re->opaque = opaque;
> > -    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
> > -}
> > -
> > -void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
> > -{
> > -    QEMUResetEntry *re;
> > -
> > -    QTAILQ_FOREACH(re, &reset_handlers, entry) {
> > -        if (re->func == func && re->opaque == opaque) {
> > -            QTAILQ_REMOVE(&reset_handlers, re, entry);
> > -            g_free(re);
> > -            return;
> > -        }
> > -    }
> > -}
> > -
> > -void qemu_devices_reset(void)
> > -{
> > -    QEMUResetEntry *re, *nre;
> > -
> > -    /* reset all devices */
> > -    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
> > -        re->func(re->opaque);
> > -    }
> > -}
> > -
> >  void qemu_system_reset(bool report)
> >  {
> >      if (current_machine && current_machine->reset) {
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo