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 <vilan...@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.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
