Re: [PATCH v24 03/10] trace-cmd: Add trace-cmd library APIs for ftrace clock name

[Steven Rostedt <rostedt@goodmis.org>]

On Fri,  9 Oct 2020 17:03:31 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> 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@gmail.com>
> ---
>  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