Re: [PATCH] libtracefs: Add support for setting tracers

* Re: [PATCH] libtracefs: Add support for setting tracers
  2021-05-16 14:59 [PATCH] libtracefs: Add support for setting tracers Sameeruddin shaik
@ 2021-05-15 16:40 ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2021-05-15 16:40 UTC (permalink / raw)
  To: Sameeruddin shaik; +Cc: linux-trace-devel

Hi Sameer,

On Sun, 16 May 2021 20:29:19 +0530
Sameeruddin shaik <sameeross1994@gmail.com> wrote:

> tracefs_set_tracer - set the tracer
> tracefs_stop_tracer - clear the tracer
> 
> Signed-off-by: Sameeruddin shaik <sameeross1994@gmail.com>
> 
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 55ee867..47d3282 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -173,4 +173,26 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char *filte
>  int tracefs_function_notrace(struct tracefs_instance *instance, const char *filter,
>  			     const char *module, unsigned int flags);
>  
> +/*
> + * Tracers
> + */
> +enum tracefs_tracers {
> +	TRACEFS_TRACER_NOP = 0,
> +	TRACEFS_TRACER_FUNCTION,
> +	TRACEFS_TRACER_FUNCTION_GRAPH,
> +	TRACEFS_TRACER_IRQSOFF,
> +	TRACEFS_TRACER_PREEMPTOFF,
> +	TRACEFS_TRACER_PREEMPTIRQSOFF,
> +	TRACEFS_TRACER_WAKEUP,
> +	TRACEFS_TRACER_WAKEUP_RT,
> +	TRACEFS_TRACER_WAKEUP_DL,
> +	TRACEFS_TRACER_MMIOTRACE,
> +	TRACEFS_TRACER_HWLAT,
> +	TRACEFS_TRACER_BRANCH,
> +	TRACEFS_TRACER_BLOCK,
> +};
> +
> +int tracefs_set_tracer(struct tracefs_instance *instance, enum tracefs_tracers tracer);
> +
> +int tracefs_stop_tracer(struct tracefs_instance *instance);
>  #endif /* _TRACE_FS_H */
> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
> index 6ef17f6..ceddeda 100644
> --- a/src/tracefs-tools.c
> +++ b/src/tracefs-tools.c
> @@ -25,6 +25,7 @@ __hidden pthread_mutex_t toplevel_lock = PTHREAD_MUTEX_INITIALIZER;
>  #define TRACE_FILTER		"set_ftrace_filter"
>  #define TRACE_NOTRACE		"set_ftrace_notrace"
>  #define TRACE_FILTER_LIST	"available_filter_functions"
> +#define TRACER			"current_tracer"
>  
>  /* File descriptor for Top level set_ftrace_filter  */
>  static int ftrace_filter_fd = -1;
> @@ -910,3 +911,88 @@ int tracefs_function_notrace(struct tracefs_instance *instance, const char *filt
>  	tracefs_put_tracing_file(filter_path);
>  	return ret;
>  }
> +
> +int write_tracer(int fd, const char *tracer)
> +{
> +	int ret;
> +
> +	ret = write(fd, tracer, strlen(tracer));
> +	if (ret < strlen(tracer))
> +		return -1;
> +	return ret;
> +}
> +
> +/**
> +* tracefs_set_tracer - fucntion to set the tracer
> +* @instance: ftrace instance, can be NULL for top tracing instance
> +* @tracer: Tracer that has to be set, which can be integer from 0 - 13
> +* or and enum value
> +*/
> +
> +int tracefs_set_tracer(struct tracefs_instance *instance, enum tracefs_tracers tracer)
> +{
> +	char *tracer_path;
> +	int ret = -1;
> +	int fd = -1;
> +
> +	tracer_path = tracefs_instance_get_file(instance, TRACER);
> +	if (!tracer_path)
> +		return -1;
> +
> +	fd = open(tracer_path, O_WRONLY);
> +	if (fd < 0)
> +		goto out;
> +
> +	switch(tracer) {
> +	case 0:
> +		ret = write_tracer(fd , "nop");
> +		break;
> +	case 1:
> +		ret = write_tracer(fd, "function");
> +		break;
> +	case 2:
> +		ret = write_tracer(fd, "function_graph");
> +		break;
> +	case 3:
> +		ret = write_tracer(fd, "irqsoff");
> +		break;
> +	case 4:
> +		ret = write_tracer(fd, "preemptoff");
> +		break;
> +	case 5:
> +		ret = write_tracer(fd, "preemptirqsoff");
> +		break;
> +	case 6:
> +		ret = write_tracer(fd, "wakeup");
> +		break;
> +	case 7:
> +		ret = write_tracer(fd, "wakeup_rt");
> +		break;
> +	case 8:
> +		ret = write_tracer(fd, "wakeup_dl");
> +		break;
> +	case 9:
> +		ret = write_tracer(fd, "mmiotrace");
> +		break;
> +	case 10:
> +		ret = write_tracer(fd, "hwlat");
> +		break;
> +	case 11:
> +		ret = write_tracer(fd, "branch");
> +		break;
> +	case 12:
> +		ret = write_tracer(fd, "blk");
> +		break;
> +	default:

Note, hardcoded numbers above is frowned upon in computer science in
general. But you already have the enums, why didn't you use them?

But besides the point, you don't even need a switch statement.

#define TRACERS \
	C(NOP,			"nop"),			\
	C(FUNCTION,		"function"),		\
	C(FUNCTION_GRAPH,	"function_graph"),	\
	C(IRQSOFF,		"irqsoff"),		\
	C(PREEMPTOFF,		"preemptoff"),		\
	C(PREEMPTIQRSOFF,	"preemptirqsoff"),	\
	C(WAKEUP,		"wakeup"),		\
	C(WAKEUP_RT,		"wakeup_rt")'		\
	C(WAKEUP_DL,		"wakeup_dl"),		\
	C(MMIOTRACE,		"mmiotrace"),		\
	C(HWLAT,		"hwlat"),		\
	C(BRANCH,		"branch"),		\
	C(BLOCK,		"block")

#undef C
#define C(a,b) b

const char *tracers[] = { TRACERS };

#undef C
#define C(a,b) TRACEFS_TRACER_##a

const int *tracer_enums[] = { TRACERS };

#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))

{
	const char *t = NULL;
	[..]

	if (tracer < 0 || tracer >= ARRAY_SIZE(tracers)) {
		errno = -ENODEV;
		return -1;
	}

	/* if we didn't make a mistake in our mapping */
	if (tracer == tracer_enums[tracer]) {
		t = tracers[tracer];
	} else {
		for (i = 0; i < ARRAY_SIZE(tracer_enums)) {
			if (tracer == tracer_enums[tracer]) {
				t = tracers[i];
				break;
			}
		}
	}

	if (!t) {
		/* Something went horribly wrong */
		errno = -EINVAL;
		return -1;
	}

	ret = write_tracer(fd, t);

Or something like the above. Much more robust, much more maintainable.

-- Steve

> +		ret = -1;
> +	}
> +
> + out:
> +	tracefs_put_tracing_file(tracer_path);
> +	return ret;
> +}
> +
> +int  tracefs_stop_tracer(struct tracefs_instance *instance)
> +{
> +	return tracefs_set_tracer(instance, TRACEFS_TRACER_NOP);
> +}

^ permalink raw reply	[flat|nested] 8+ messages in thread