On Fri, 9 Oct 2020 17:03:31 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> Added enum with ftrace clock IDs and APIs to convert ftrace name to ID
> and vice versa, as part of libtracecmd.
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
> include/trace-cmd/trace-cmd.h | 16 +++++++++++++
> lib/trace-cmd/trace-util.c | 45 +++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index f9c1f843..393a2e7b 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -415,6 +415,22 @@ int tracecmd_msg_recv_time_sync(struct tracecmd_msg_handle *msg_handle,
> unsigned int *sync_msg_id,
> unsigned int *payload_size, char **payload);
>
> +enum tracecmd_clocks {
> + TRACECMD_CLOCK_UNKNOWN = 0,
> + TRACECMD_CLOCK_LOCAL = 1,
> + TRACECMD_CLOCK_GLOBAL = 1 << 1,
> + TRACECMD_CLOCK_COUNTER = 1 << 2,
> + TRACECMD_CLOCK_UPTIME = 1 << 3,
> + TRACECMD_CLOCK_PERF = 1 << 4,
> + TRACECMD_CLOCK_MONO = 1 << 5,
> + TRACECMD_CLOCK_MONO_RAW = 1 << 6,
> + TRACECMD_CLOCK_BOOT = 1 << 7,
> + TRACECMD_CLOCK_X86_TSC = 1 << 8
I'm curious to why you have this as a bitmask. We can only have on
clock at a time, right?
Also, if we make it a simple counter, we can create a string as well:
#define TRACECMD_CLOCKS \
C(UNKNOWN, unknown), \
C(LOCAL, local), \
C(GLOBAL, global),\
C(COUNTER, counter),\
C(UPTIME, uptime),\
C(PERF, perf), \
C(MONO, mono), \
C(MONO_RAW, mono_raw),\
C(BOOT, boot, \
C(X86_TSC, x86-tsc)
#undef C
#define C(a, b) TRACECMD_CLOCK_##a
enum tracecmd_clocks { TRACECMD_CLOCKS };
#undef C
> +};
> +
> +enum tracecmd_clocks tracecmd_clock_str2id(const char *clock);
> +char *tracecmd_clock_id2str(enum tracecmd_clocks clock);
> +
> /* --- Timestamp synchronization --- */
>
> enum{
> diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> index 0ead96ea..e20362e3 100644
> --- a/lib/trace-cmd/trace-util.c
> +++ b/lib/trace-cmd/trace-util.c
> @@ -33,6 +33,51 @@ static bool debug;
>
> static FILE *logfp;
>
> +const static struct {
> + char *clock_str;
> + enum tracecmd_clocks clock_id;
> +} trace_clocks[] = {
> + {"local", TRACECMD_CLOCK_LOCAL},
> + {"global", TRACECMD_CLOCK_GLOBAL},
> + {"counter", TRACECMD_CLOCK_COUNTER},
> + {"uptime", TRACECMD_CLOCK_UPTIME},
> + {"perf", TRACECMD_CLOCK_PERF},
> + {"mono", TRACECMD_CLOCK_MONO},
> + {"mono_raw", TRACECMD_CLOCK_MONO_RAW},
> + {"boot", TRACECMD_CLOCK_BOOT},
> + {"x86-tsc", TRACECMD_CLOCK_X86_TSC},
> + {NULL, -1}
> +};
He we would have:
#define C(a, b) #b
const char * trace_clocks[] = { TRACECMD_CLOCKS }
> +
> +/**
> + * tracecmd_clock_str2id - Convert ftrace clock name to clock ID
> + * @clock: Ftrace clock name
> + * Returns ID of the ftrace clock
> + */
> +enum tracecmd_clocks tracecmd_clock_str2id(const char *clock)
> +{
int i;
btw, in normal C, it's not recommended to declare a counter in a loop.
for (i = 0; i < ARRAY_SIZE(trace_clocks); i++) {
if (strcmp(trace_clocks[i], clock) == 0)
return i;
return 0;
}
> + for (int i = 0; trace_clocks[i].clock_str; i++) {
> + if (!strncmp(clock, trace_clocks[i].clock_str,
> + strlen(trace_clocks[i].clock_str)))
> + return trace_clocks[i].clock_id;
> + }
> + return TRACECMD_CLOCK_UNKNOWN;
> +}
> +
> +/**
> + * tracecmd_clock_id2str - Convert clock ID to ftare clock name
> + * @clock: Clock ID
> + * Returns name of a ftrace clock
> + */
> +char *tracecmd_clock_id2str(enum tracecmd_clocks clock)
Should probably have this return const char *.
Such that callers wont modify it.
> +{
And here we would have:
if (clock < ARRAY_SIZE(trace_clocks))
return trace_clocks[i];
return NULL;
-- Steve
> + for (int i = 0; trace_clocks[i].clock_str; i++) {
> + if (trace_clocks[i].clock_id == clock)
> + return trace_clocks[i].clock_str;
> + }
> + return NULL;
> +}
> +
> /**
> * tracecmd_set_debug - Set debug mode of the tracecmd library
> * @set_debug: The new "debug" mode. If true, the tracecmd library is
|