From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXp2O-0007zk-Ds for qemu-devel@nongnu.org; Thu, 11 Aug 2016 08:24:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXp2K-0004EF-5l for qemu-devel@nongnu.org; Thu, 11 Aug 2016 08:24:39 -0400 Received: from roura.ac.upc.edu ([147.83.33.10]:58438 helo=roura.ac.upc.es) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXp2J-0004CW-Qn for qemu-devel@nongnu.org; Thu, 11 Aug 2016 08:24:36 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <147091098568.31540.12781119248124146421.stgit@fimbulvetr.bsc.es> <20160811102831.GL27458@redhat.com> <878tw3lg9d.fsf@fimbulvetr.bsc.es> <20160811112113.GT27458@redhat.com> Date: Thu, 11 Aug 2016 14:24:31 +0200 In-Reply-To: <20160811112113.GT27458@redhat.com> (Daniel P. Berrange's message of "Thu, 11 Aug 2016 12:21:13 +0100") Message-ID: <87lh03jxxc.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 01:03:10PM +0200, Llu=C3=ADs Vilanova wrote: >> Daniel P Berrange writes: >>=20 >> > 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 on= ly >> >> 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]; >>=20 >> > This is doing arithmetic on a boolean value >>=20 >> >> + trace_events_dstate[id] =3D state; >>=20 >> > This is assigning a boolean value to an int16_t >>=20 >> >> void trace_event_set_state_dynamic(TraceEvent *ev, bool state) >> >> { >> >> CPUState *vcpu; >>=20 >>=20 >> Mmmm, all these were c&p from existing code, just moved to a new functio= n. I do >> remember explicitly checking for the boolean values with if/else to do t= he >> appropriate operations, but someone reviewing the code suggested using w= hat >> you're seeing to make it shorter and more readable. > I think this logic is considerably less understandable with this magic > trick, because it is not at all obvious that trace_events_dstate[id] > will always be 0 if state is True. I agree. I can change it if noone else objects (Stefan?), but the rest of t= he tracing code uses this shorter approach. Cheers, Lluis