Re: [Qemu-devel] [PATCH v2] trace: Remove 'trace_events_dstate_init'

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Daniel P Berrange writes:

> On Thu, Aug 11, 2016 at 01:03:10PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>> 
>> > On Thu, Aug 11, 2016 at 12:23:05PM +0200, Lluís 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.
>> >> 
>> >> Assumes that the user won't touch the state of "vcpu" events between
>> >> early and late initialization (e.g., through QMP).
>> >> 
>> >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> >> ---
>> >> 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(-)
>> >> 
>> >> 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"
>> >> 
>> >> 
>> >> +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"
>> >> 
>> >> 
>> >> +void trace_event_set_state_dynamic_init(TraceEvent *ev, bool state)
>> >> +{
>> >> +    TraceEventID id = 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 += state - trace_events_dstate[id];
>> 
>> > This is doing arithmetic on a boolean value
>> 
>> >> +    trace_events_dstate[id] = 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.

> 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 the
tracing code uses this shorter approach.

Cheers,
  Lluis