qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Lluís Vilanova
Subject: Re: [Qemu-devel] [PATCH v2] trace: Remove 'trace_events_dstate_init'
Date: Thu, 11 Aug 2016 13:03:10 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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 <address@hidden>
>> ---
>> 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.

Bottom line, the same reasons that led to this code style should hold here too.

PS: I don't know what the standard says about these implicit conversions.

Cheers,
  Lluis



reply via email to

[Prev in Thread] Current Thread [Next in Thread]