diff mbox

[for-2.8,v1,11/60] trace: remove use of event ID enums from APIs

Message ID 1470756748-18933-12-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Aug. 9, 2016, 3:31 p.m. UTC
Since there will shortly be multiple event groups allowed,
we can no longer use the TraceEventID and TraceEventVCPUID
enums in the trace control APIs. There will in fact be
multiple distinct enums, and the enum values will only be
required to be unique per group.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 trace/control-internal.h | 14 +++++++-------
 trace/control-target.c   |  6 +++---
 trace/control.h          | 25 ++++++-------------------
 trace/event-internal.h   |  4 ++--
 trace/simple.c           |  6 +++---
 trace/simple.h           |  2 +-
 6 files changed, 22 insertions(+), 35 deletions(-)

Comments

Paolo Bonzini Aug. 9, 2016, 4:18 p.m. UTC | #1
On 09/08/2016 17:31, Daniel P. Berrange wrote:
> Since there will shortly be multiple event groups allowed,
> we can no longer use the TraceEventID and TraceEventVCPUID
> enums in the trace control APIs. There will in fact be
> multiple distinct enums, and the enum values will only be
> required to be unique per group.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  trace/control-internal.h | 14 +++++++-------
>  trace/control-target.c   |  6 +++---
>  trace/control.h          | 25 ++++++-------------------
>  trace/event-internal.h   |  4 ++--
>  trace/simple.c           |  6 +++---
>  trace/simple.h           |  2 +-
>  6 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/trace/control-internal.h b/trace/control-internal.h
> index 634effe..b7048d4 100644
> --- a/trace/control-internal.h
> +++ b/trace/control-internal.h
> @@ -24,13 +24,13 @@ static inline bool trace_event_is_pattern(const char *str)
>      return strchr(str, '*') != NULL;
>  }
>  
> -static inline TraceEventID trace_event_get_id(TraceEvent *ev)
> +static inline size_t trace_event_get_id(TraceEvent *ev)
>  {
>      assert(ev != NULL);
>      return ev->id;

Perhaps "unsigned" is a better match than size_t?

Paolo
Daniel P. Berrangé Aug. 9, 2016, 4:24 p.m. UTC | #2
On Tue, Aug 09, 2016 at 06:18:51PM +0200, Paolo Bonzini wrote:
> 
> 
> On 09/08/2016 17:31, Daniel P. Berrange wrote:
> > Since there will shortly be multiple event groups allowed,
> > we can no longer use the TraceEventID and TraceEventVCPUID
> > enums in the trace control APIs. There will in fact be
> > multiple distinct enums, and the enum values will only be
> > required to be unique per group.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  trace/control-internal.h | 14 +++++++-------
> >  trace/control-target.c   |  6 +++---
> >  trace/control.h          | 25 ++++++-------------------
> >  trace/event-internal.h   |  4 ++--
> >  trace/simple.c           |  6 +++---
> >  trace/simple.h           |  2 +-
> >  6 files changed, 22 insertions(+), 35 deletions(-)
> > 
> > diff --git a/trace/control-internal.h b/trace/control-internal.h
> > index 634effe..b7048d4 100644
> > --- a/trace/control-internal.h
> > +++ b/trace/control-internal.h
> > @@ -24,13 +24,13 @@ static inline bool trace_event_is_pattern(const char *str)
> >      return strchr(str, '*') != NULL;
> >  }
> >  
> > -static inline TraceEventID trace_event_get_id(TraceEvent *ev)
> > +static inline size_t trace_event_get_id(TraceEvent *ev)
> >  {
> >      assert(ev != NULL);
> >      return ev->id;
> 
> Perhaps "unsigned" is a better match than size_t?

I don't mind either way - I just happen to personally always use size_t
for anything that ends up being used primarily as an array index.

Regards,
Daniel
Paolo Bonzini Aug. 9, 2016, 4:26 p.m. UTC | #3
On 09/08/2016 18:24, Daniel P. Berrange wrote:
>>> > > -static inline TraceEventID trace_event_get_id(TraceEvent *ev)
>>> > > +static inline size_t trace_event_get_id(TraceEvent *ev)
>>> > >  {
>>> > >      assert(ev != NULL);
>>> > >      return ev->id;
>> > 
>> > Perhaps "unsigned" is a better match than size_t?
> I don't mind either way - I just happen to personally always use size_t
> for anything that ends up being used primarily as an array index.

Makes sense.  I was thinking of simpletrace's 32-bit id instead.

Paolo
Stefan Hajnoczi Sept. 2, 2016, 9:13 p.m. UTC | #4
On Tue, Aug 09, 2016 at 06:26:49PM +0200, Paolo Bonzini wrote:
> 
> 
> On 09/08/2016 18:24, Daniel P. Berrange wrote:
> >>> > > -static inline TraceEventID trace_event_get_id(TraceEvent *ev)
> >>> > > +static inline size_t trace_event_get_id(TraceEvent *ev)
> >>> > >  {
> >>> > >      assert(ev != NULL);
> >>> > >      return ev->id;
> >> > 
> >> > Perhaps "unsigned" is a better match than size_t?
> > I don't mind either way - I just happen to personally always use size_t
> > for anything that ends up being used primarily as an array index.
> 
> Makes sense.  I was thinking of simpletrace's 32-bit id instead.

I think unsigned is slightly clearer since it expresses the intent that
the values are limited to 32 bits.

Stefan
Daniel P. Berrangé Sept. 14, 2016, 12:32 p.m. UTC | #5
On Fri, Sep 02, 2016 at 05:13:48PM -0400, Stefan Hajnoczi wrote:
> On Tue, Aug 09, 2016 at 06:26:49PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 09/08/2016 18:24, Daniel P. Berrange wrote:
> > >>> > > -static inline TraceEventID trace_event_get_id(TraceEvent *ev)
> > >>> > > +static inline size_t trace_event_get_id(TraceEvent *ev)
> > >>> > >  {
> > >>> > >      assert(ev != NULL);
> > >>> > >      return ev->id;
> > >> > 
> > >> > Perhaps "unsigned" is a better match than size_t?
> > > I don't mind either way - I just happen to personally always use size_t
> > > for anything that ends up being used primarily as an array index.
> > 
> > Makes sense.  I was thinking of simpletrace's 32-bit id instead.
> 
> I think unsigned is slightly clearer since it expresses the intent that
> the values are limited to 32 bits.

I'll switch to 'uint32_t' since that unambigously matches that
simpletrace mandates.


Regards,
Daniel
diff mbox

Patch

diff --git a/trace/control-internal.h b/trace/control-internal.h
index 634effe..b7048d4 100644
--- a/trace/control-internal.h
+++ b/trace/control-internal.h
@@ -24,13 +24,13 @@  static inline bool trace_event_is_pattern(const char *str)
     return strchr(str, '*') != NULL;
 }
 
-static inline TraceEventID trace_event_get_id(TraceEvent *ev)
+static inline size_t trace_event_get_id(TraceEvent *ev)
 {
     assert(ev != NULL);
     return ev->id;
 }
 
-static inline TraceEventVCPUID trace_event_get_vcpu_id(TraceEvent *ev)
+static inline size_t trace_event_get_vcpu_id(TraceEvent *ev)
 {
     return ev->vcpu_id;
 }
@@ -53,7 +53,7 @@  static inline bool trace_event_get_state_static(TraceEvent *ev)
 }
 
 static inline bool trace_event_get_state_dynamic_by_id(
-    uint16_t *trace_events_dstate, TraceEventID id)
+    uint16_t *trace_events_dstate, size_t id)
 {
     /* it's on fast path, avoid consistency checks (asserts) */
     return unlikely(trace_events_enabled_count) && trace_events_dstate[id];
@@ -62,14 +62,14 @@  static inline bool trace_event_get_state_dynamic_by_id(
 static inline bool trace_event_get_state_dynamic(
     uint16_t *trace_events_dstate, TraceEvent *ev)
 {
-    TraceEventID id;
+    size_t id;
     assert(trace_event_get_state_static(ev));
     id = trace_event_get_id(ev);
     return trace_event_get_state_dynamic_by_id(trace_events_dstate, id);
 }
 
-static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(
-    CPUState *vcpu, TraceEventVCPUID id)
+static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu,
+                                                                 size_t id)
 {
     /* it's on fast path, avoid consistency checks (asserts) */
     if (unlikely(trace_events_enabled_count)) {
@@ -82,7 +82,7 @@  static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(
 static inline bool trace_event_get_vcpu_state_dynamic(CPUState *vcpu,
                                                       TraceEvent *ev)
 {
-    TraceEventVCPUID id;
+    size_t id;
     assert(trace_event_is_vcpu(ev));
     id = trace_event_get_vcpu_id(ev);
     return trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, id);
diff --git a/trace/control-target.c b/trace/control-target.c
index da326b4..2f5a2c2 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -22,7 +22,7 @@  void trace_event_set_state_dynamic(uint16_t *dstate, TraceEvent *ev, bool state)
             trace_event_set_vcpu_state_dynamic(dstate, vcpu, ev, state);
         }
     } else {
-        TraceEventID id = trace_event_get_id(ev);
+        size_t id = trace_event_get_id(ev);
         trace_events_enabled_count += state - dstate[id];
         dstate[id] = state;
     }
@@ -31,8 +31,8 @@  void trace_event_set_state_dynamic(uint16_t *dstate, TraceEvent *ev, bool state)
 void trace_event_set_vcpu_state_dynamic(uint16_t *dstate, CPUState *vcpu,
                                         TraceEvent *ev, bool state)
 {
-    TraceEventID id;
-    TraceEventVCPUID vcpu_id;
+    size_t id;
+    size_t vcpu_id;
     bool state_pre;
     assert(trace_event_get_state_static(ev));
     assert(trace_event_is_vcpu(ev));
diff --git a/trace/control.h b/trace/control.h
index a5cb7f3..438c3d2 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -19,19 +19,6 @@  typedef struct TraceEventIter {
     const char *pattern;
 } TraceEventIter;
 
-
-/**
- * TraceEventID:
- *
- * Unique tracing event identifier.
- *
- * These are named as 'TRACE_${EVENT_NAME}'.
- *
- * See also: "trace/generated-events.h"
- */
-enum TraceEventID;
-
-
 void trace_event_iter_init(TraceEventIter *iter, const char *pattern);
 
 TraceEvent *trace_event_iter_next(TraceEventIter *iter);
@@ -63,7 +50,7 @@  static bool trace_event_is_pattern(const char *str);
  *
  * Get the identifier of an event.
  */
-static TraceEventID trace_event_get_id(TraceEvent *ev);
+static size_t trace_event_get_id(TraceEvent *ev);
 
 /**
  * trace_event_get_vcpu_id:
@@ -73,7 +60,7 @@  static TraceEventID trace_event_get_id(TraceEvent *ev);
  * Special value #TRACE_VCPU_EVENT_COUNT means the event is not vCPU-specific
  * (does not have the "vcpu" property).
  */
-static TraceEventVCPUID trace_event_get_vcpu_id(TraceEvent *ev);
+static size_t trace_event_get_vcpu_id(TraceEvent *ev);
 
 /**
  * trace_event_is_vcpu:
@@ -98,7 +85,7 @@  static const char * trace_event_get_name(TraceEvent *ev);
  * If the event has the disabled property, the check will have no performance
  * impact.
  *
- * As a down side, you must always use an immediate #TraceEventID value.
+ * As a down side, you must always use an immediate event ID value.
  */
 #define trace_event_get_state(dstate, id)                                \
     ((id ##_ENABLED) && trace_event_get_state_dynamic_by_id(dstate, id))
@@ -106,8 +93,8 @@  static const char * trace_event_get_name(TraceEvent *ev);
 /**
  * trace_event_get_vcpu_state:
  * @vcpu: Target vCPU.
- * @id: Event identifier (TraceEventID).
- * @vcpu_id: Per-vCPU event identifier (TraceEventVCPUID).
+ * @id: Event identifier.
+ * @vcpu_id: Per-vCPU event identifier.
  *
  * Get the tracing state of an event (both static and dynamic) for the given
  * vCPU.
@@ -115,7 +102,7 @@  static const char * trace_event_get_name(TraceEvent *ev);
  * If the event has the disabled property, the check will have no performance
  * impact.
  *
- * As a down side, you must always use an immediate #TraceEventID value.
+ * As a down side, you must always use an immediate event ID value.
  */
 #define trace_event_get_vcpu_state(vcpu, id, vcpu_id)                   \
     ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, vcpu_id))
diff --git a/trace/event-internal.h b/trace/event-internal.h
index 5d8fa97..618d1b5 100644
--- a/trace/event-internal.h
+++ b/trace/event-internal.h
@@ -23,8 +23,8 @@ 
  * Opaque generic description of a tracing event.
  */
 typedef struct TraceEvent {
-    TraceEventID id;
-    TraceEventVCPUID vcpu_id;
+    size_t id;
+    size_t vcpu_id;
     const char * name;
     const bool sstate;
 } TraceEvent;
diff --git a/trace/simple.c b/trace/simple.c
index 2f09daf..2338ff8 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -18,7 +18,7 @@ 
 #include "trace/simple.h"
 
 /** Trace file header event ID */
-#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */
+#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with event IDs */
 
 /** Trace file magic number */
 #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL
@@ -58,7 +58,7 @@  static char *trace_file_name;
 
 /* * Trace buffer entry */
 typedef struct {
-    uint64_t event; /*   TraceEventID */
+    uint64_t event; /*  event ID */
     uint64_t timestamp_ns;
     uint32_t length;   /*    in bytes */
     uint32_t pid;
@@ -202,7 +202,7 @@  void trace_record_write_str(TraceBufferRecord *rec, const char *s, uint32_t slen
     rec->rec_off = write_to_buffer(rec->rec_off, (void*)s, slen);
 }
 
-int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize)
+int trace_record_start(TraceBufferRecord *rec, size_t event, size_t datasize)
 {
     unsigned int idx, rec_off, old_idx, new_idx;
     uint32_t rec_len = sizeof(TraceRecord) + datasize;
diff --git a/trace/simple.h b/trace/simple.h
index 1e7de45..862bbc7 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -33,7 +33,7 @@  typedef struct {
  *
  * @arglen  number of bytes required for arguments
  */
-int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen);
+int trace_record_start(TraceBufferRecord *rec, size_t id, size_t arglen);
 
 /**
  * Append a 64-bit argument to a trace record