Re: [Qemu-devel] [PATCH v3 2/2] trace: Avoid implicit bool->integer conversions

[Lluís Vilanova]

Daniel P Berrange writes:

> On Thu, Aug 11, 2016 at 06:41:36PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>> 
>> > On Thu, Aug 11, 2016 at 06:11:29PM +0200, Lluís Vilanova wrote:
>> >> An explicit if/else is clearer than arithmetic assuming #true is 1,
>> >> while the compiler should be able to generate just as optimal code.
>> >> 
>> >> Signed-off-by: Lluís Vilanova <address@hidden>
>> >> ---
>> >> stubs/trace-control.c  |    9 +++++++--
>> >> trace/control-target.c |   18 ++++++++++++++----
>> >> 2 files changed, 21 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/stubs/trace-control.c b/stubs/trace-control.c
>> >> index 3740c38..099c2b5 100644
>> >> --- a/stubs/trace-control.c
>> >> +++ b/stubs/trace-control.c
>> >> @@ -21,8 +21,13 @@ void trace_event_set_state_dynamic(TraceEvent *ev, 
>> >> bool state)
>> >> TraceEventID id;
>> >> assert(trace_event_get_state_static(ev));
>> >> id = trace_event_get_id(ev);
>> >> -    trace_events_enabled_count += state - trace_events_dstate[id];
>> >> -    trace_events_dstate[id] = state;
>> >> +    if (state) {
>> >> +        trace_events_enabled_count++;
>> >> +        trace_events_dstate[id] = 1;
>> >> +    } else {
>> >> +        trace_events_enabled_count--;
>> 
>> > SHouldn't this be trace_events_enabled_count -= trace_events_dstate[id];
>> > as it is possible to call this method for a trace event that is a per-vcpu
>> > event
>> 
>> The "stub" code will never have vCPUs around, it's used by qemu-img and the 
>> like
>> (any program without "target" code).

> I just picked the first example of that in this file - you've got the same
> pattern in the other 2 places you change

In the "control-target.c" file, there's two places where this pattern also
appears.

In trace_event_set_state_dynamic(), the pattern only appears on events that do
*not* have the "vcpu" property, and so the counter is always either 1 or 0.

In trace_event_set_state_dynamic_init() there's the "special" case where we
assume there won't be multiple enables of a single event with "vcpu" (as the
comment says); this is what let's us get rid of the dstate_init array.

What I just saw is wrong is that this code looses the ability to make
enable/disable operations idempotent. I'll send a v4 with that fixed.


Thanks,
  Lluis