From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52876) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXnlg-0007AN-1h for qemu-devel@nongnu.org; Thu, 11 Aug 2016 07:03:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXnlZ-0001SJ-VT for qemu-devel@nongnu.org; Thu, 11 Aug 2016 07:03:18 -0400 Received: from roura.ac.upc.es ([147.83.33.10]:50248) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXnlZ-0001Rj-KJ for qemu-devel@nongnu.org; Thu, 11 Aug 2016 07:03:13 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <147091098568.31540.12781119248124146421.stgit@fimbulvetr.bsc.es> <20160811102831.GL27458@redhat.com> Date: Thu, 11 Aug 2016 13:03:10 +0200 In-Reply-To: <20160811102831.GL27458@redhat.com> (Daniel P. Berrange's message of "Thu, 11 Aug 2016 11:28:31 +0100") Message-ID: <878tw3lg9d.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] trace: Remove 'trace_events_dstate_init' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Daniel P Berrange writes: > On Thu, Aug 11, 2016 at 12:23:05PM +0200, Llu=C3=ADs Vilanova wrote: >> Removes the event state array used for early initialization. Since only >> events with the "vcpu" property need a late initialization fixup, >> threats their initialization specially. >>=20 >> Assumes that the user won't touch the state of "vcpu" events between >> early and late initialization (e.g., through QMP). >>=20 >> Signed-off-by: Llu=C3=ADs Vilanova >> --- >> stubs/trace-control.c | 5 +++++ >> trace/control-target.c | 9 +++++++++ >> trace/control.c | 23 ++++++++++------------- >> trace/control.h | 3 +++ >> trace/event-internal.h | 1 + >> 5 files changed, 28 insertions(+), 13 deletions(-) >>=20 >> diff --git a/stubs/trace-control.c b/stubs/trace-control.c >> index fe59836..3740c38 100644 >> --- a/stubs/trace-control.c >> +++ b/stubs/trace-control.c >> @@ -11,6 +11,11 @@ >> #include "trace/control.h" >>=20 >>=20 >> +void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state) >> +{ >> + trace_event_set_state_dynamic(ev, state); >> +} >> + >> void trace_event_set_state_dynamic(TraceEvent *ev, bool state) >> { >> TraceEventID id; >> diff --git a/trace/control-target.c b/trace/control-target.c >> index 74c029a..4ee3733 100644 >> --- a/trace/control-target.c >> +++ b/trace/control-target.c >> @@ -13,6 +13,15 @@ >> #include "translate-all.h" >>=20 >>=20 >> +void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state) >> +{ >> + TraceEventID id =3D trace_event_get_id(ev); >> + assert(trace_event_get_state_static(ev)); >> + /* Ignore "vcpu" property, since no vCPUs have been created yet */ >> + trace_events_enabled_count +=3D state - trace_events_dstate[id]; > This is doing arithmetic on a boolean value >> + trace_events_dstate[id] =3D state; > This is assigning a boolean value to an int16_t >> void trace_event_set_state_dynamic(TraceEvent *ev, bool state) >> { >> CPUState *vcpu; Mmmm, all these were c&p from existing code, just moved to a new function. = I do remember explicitly checking for the boolean values with if/else to do the appropriate operations, but someone reviewing the code suggested using what you're seeing to make it shorter and more readable. Bottom line, the same reasons that led to this code style should hold here = too. PS: I don't know what the standard says about these implicit conversions. Cheers, Lluis