Re: [Qemu-devel] [PATCH v2 5/6] trace: remove use of event ID enums from APIs

[Lluís Vilanova]

Daniel P Berrange writes:

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

This patch serves no purpose without the event group patches.

Also, AFAIR TraceEventVCPUID still needs to be a flat space (they're all used as
bitmask indexes), so keeping the enum won't lose any re-compilation benefit.

And without wanting to sound like a broken record, you can make the
"TRACE_${EVENTNAME}" IDs be global Event* variables (statically initialized in
"trace/generated-events.c"). That still allows using their names in the macros,
avoids having a (two-level) tree of events, and eliminates the need for the
Event::id member (and the trace_event_get_id() function).


Cheers,
  Lluis


[...]
> diff --git a/trace/simple.c b/trace/simple.c
> index 2f09daf..6e8013c 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, uint32_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..17ce472 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, uint32_t id, size_t arglen);
 
>  /**
>   * Append a 64-bit argument to a trace record

Not incorrect, but it's weird that the simple backend emits 64-bit identifiers
while QEMU uses 32-bit ones.


Cheers,
  Lluis