Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Paolo Bonzini <pbonzini@redhat.com>]

On 25/06/2015 04:17, Zhu Guihua wrote:
> Add a wrapper to specify reset order when registering reset handler,
> instead of non-obvious initiazation code ordering.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

I'm sorry, this is not really acceptable.  The initialization code
ordering is good because it should be okay to run reset handlers in the
same order as code is run.  If there are dependencies between reset
handlers, a random integer is not a maintainable way to maintain them.

Instead, you should have a single reset handler that calls the reset
handlers in the right order; for example a qdev bus such as icc_bus
always resets children before parents.

Are you sure that you want to remove the icc_bus?... What are you
gaining exactly by doing so?

Paolo

> ---
>  include/hw/hw.h |  4 ++++
>  vl.c            | 18 +++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/hw.h b/include/hw/hw.h
> index c78adae..d9375e7 100644
> --- a/include/hw/hw.h
> +++ b/include/hw/hw.h
> @@ -37,7 +37,11 @@
>  #endif
>  
>  typedef void QEMUResetHandler(void *opaque);
> +typedef uint64_t QEMUResetOrder;
> +#define default_reset_order 0x0
>  
> +void qemu_register_reset_common(QEMUResetHandler *func, void *opaque,
> +                                QEMUResetOrder reset_order);
>  void qemu_register_reset(QEMUResetHandler *func, void *opaque);
>  void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>  
> diff --git a/vl.c b/vl.c
> index 69ad90c..b205a9b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1589,6 +1589,7 @@ typedef struct QEMUResetEntry {
>      QTAILQ_ENTRY(QEMUResetEntry) entry;
>      QEMUResetHandler *func;
>      void *opaque;
> +    QEMUResetOrder reset_order;
>  } QEMUResetEntry;
>  
>  static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> @@ -1672,15 +1673,30 @@ static int qemu_debug_requested(void)
>      return r;
>  }
>  
> -void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> +void qemu_register_reset_common(QEMUResetHandler *func, void *opaque,
> +                                QEMUResetOrder reset_order)
>  {
> +    QEMUResetEntry *item;
>      QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
>  
>      re->func = func;
>      re->opaque = opaque;
> +    re->reset_order = reset_order;
> +
> +    QTAILQ_FOREACH(item, &reset_handlers, entry) {
> +        if (re->reset_order >= item->reset_order)
> +            continue;
> +        QTAILQ_INSERT_BEFORE(item, re, entry);
> +        return;
> +    }
>      QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
>  }
>  
> +void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_register_reset_common(func, opaque, default_reset_order);
> +}
> +
>  void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
>  {
>      QEMUResetEntry *re;
>