Re: [Qemu-devel] [PATCH v4 08/17] trace: remove the TraceEventID and TraceEventVCPUID enums

[Daniel P. Berrange]

On Thu, Sep 22, 2016 at 02:35:38PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> 
> > The TraceEventID and TraceEventVCPUID enums constants are
> > no longer actually used for anything critical.
> 
> > The TRACE_EVENT_COUNT limit is used to determine the size
> > of the TraceEvents array, and can be removed if we just
> > NULL terminate the array instead.
> 
> > The TRACE_VCPU_EVENT_COUNT limit is used as a magic value
> > for marking non-vCPU events, and also for declaring the
> > size of the trace dstate mask in the CPUState struct.
> > The former usage can be replaced by a dedicated constant
> > TRACE_EVENT_VCPU_NONE, defined as (uint32_t)-1. For the
> > latter usage, we can simply define a constant for the
> > number of VCPUs, avoiding the need for the full enum.
> 
> > The only other usages of the enum values can be replaced
> > by accesing the id/vcpu_id fields via the named TraceEvent
> > structs.
> 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> 
> Disregarding comment below:
> 
> Reviewed-by: Lluís Vilanova <address@hidden>
> 
> > ---
> >  scripts/tracetool/backend/simple.py  |  4 ++--
> >  scripts/tracetool/format/events_c.py | 16 +++++++++++-----
> >  scripts/tracetool/format/events_h.py | 18 +++---------------
> >  scripts/tracetool/format/h.py        |  3 +--
> >  trace/control-internal.h             | 19 ++++++++++---------
> >  trace/control-target.c               |  2 +-
> >  trace/control.c                      |  2 +-
> >  trace/control.h                      | 31 ++++++++-----------------------
> >  trace/event-internal.h               |  6 ++++++
> >  trace/simple.c                       |  8 ++++----
> >  trace/simple.h                       |  2 +-
> >  11 files changed, 48 insertions(+), 63 deletions(-)

> > diff --git a/scripts/tracetool/format/events_h.py 
> > b/scripts/tracetool/format/events_h.py
> > index 80a66c5..5da1d4c 100644
> > --- a/scripts/tracetool/format/events_h.py
> > +++ b/scripts/tracetool/format/events_h.py
> > @@ -29,27 +29,15 @@ def generate(events, backend):
> >          out('extern TraceEvent %(event)s;',
> >              event = e.api(e.QEMU_EVENT))
>  
> > -    # event identifiers
> > -    out('typedef enum {')
> > -
> > -    for e in events:
> > -        out('    TRACE_%s,' % e.name.upper())
> > -
> > -    out('    TRACE_EVENT_COUNT',
> > -        '} TraceEventID;')
> > -
> >      for e in events:
> >          out('extern uint16_t %s;' % e.api(e.QEMU_DSTATE))
>  
> > -    # per-vCPU event identifiers
> > -    out('typedef enum {')
> > -
> > +    numvcpu = 0
> >      for e in events:
> >          if "vcpu" in e.properties:
> > -            out('    TRACE_VCPU_%s,' % e.name.upper())
> > +            numvcpu += 1
>  
> > -    out('    TRACE_VCPU_EVENT_COUNT',
> > -        '} TraceEventVCPUID;')
> 
> Here's a more pythonic way to write it:
> 
>     numvcpu = len([e for e in events if "vcpu" in e.properties])

FWIW I was tending to avoid this kind of idiom, since most of QEMU
maintainers are C developers, for whom this looks rather alien.


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 :|