From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXo37-000237-FF for qemu-devel@nongnu.org; Thu, 11 Aug 2016 07:21:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXo33-0005dE-6y for qemu-devel@nongnu.org; Thu, 11 Aug 2016 07:21:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57070) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXo32-0005d2-VG for qemu-devel@nongnu.org; Thu, 11 Aug 2016 07:21:17 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8F4598553E for ; Thu, 11 Aug 2016 11:21:16 +0000 (UTC) Date: Thu, 11 Aug 2016 12:21:13 +0100 From: "Daniel P. Berrange" Message-ID: <20160811112113.GT27458@redhat.com> Reply-To: "Daniel P. Berrange" References: <147091098568.31540.12781119248124146421.stgit@fimbulvetr.bsc.es> <20160811102831.GL27458@redhat.com> <878tw3lg9d.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <878tw3lg9d.fsf@fimbulvetr.bsc.es> 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: qemu-devel@nongnu.org, Stefan Hajnoczi 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 o= nly > >> 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 functi= on. 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. 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. Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|