From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56583) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8AWF-0007wd-2Z for qemu-devel@nongnu.org; Thu, 25 Jun 2015 13:01:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8AWB-0002XV-Ru for qemu-devel@nongnu.org; Thu, 25 Jun 2015 13:00:54 -0400 Received: from mail-wg0-x22b.google.com ([2a00:1450:400c:c00::22b]:35531) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8AWB-0002XG-CE for qemu-devel@nongnu.org; Thu, 25 Jun 2015 13:00:51 -0400 Received: by wgbhy7 with SMTP id hy7so67866473wgb.2 for ; Thu, 25 Jun 2015 10:00:50 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <558C33BE.8020603@redhat.com> Date: Thu, 25 Jun 2015 19:00:46 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <7d54c33fa2a928fd317f3a95af61f854f43ba23c.1435195913.git.zhugh.fnst@cn.fujitsu.com> In-Reply-To: <7d54c33fa2a928fd317f3a95af61f854f43ba23c.1435195913.git.zhugh.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhu Guihua , qemu-devel@nongnu.org, imammedo@redhat.com, afaerber@suse.de, ehabkost@redhat.com Cc: chen.fan.fnst@cn.fujitsu.com, izumi.taku@jp.fujitsu.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 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; >