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 <vilan...@ac.upc.edu>
>> >> ---
>> >> 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
