Re: [dpdk-dev] [PATCH v1 05/32] eal/trace: add internal trace init and fini interface

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2020-03-18 20:02, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
>
> Define eal_trace_init() and eal_trace_fini() EAL interface
> functions that rte_eal_init() and rte_eal_cleanup() function can be
> use to initialize and finalize the trace subsystem.
> eal_trace_init() function will add the following
> functionality if trace is enabled through EAL command line param.
>
> - Test for trace registration failure.
> - Test for duplicate trace name registration
> - Generate UUID ver 4.
> - Create a trace directory
>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> ---
>   lib/librte_eal/common/eal_common_trace.c      |  54 ++++++
>   .../common/eal_common_trace_utils.c           | 173 ++++++++++++++++++
>   lib/librte_eal/common/eal_trace.h             |  21 +++
>   lib/librte_eal/common/meson.build             |   1 +
>   lib/librte_eal/freebsd/eal/Makefile           |   1 +
>   lib/librte_eal/linux/eal/Makefile             |   1 +
>   6 files changed, 251 insertions(+)
>   create mode 100644 lib/librte_eal/common/eal_common_trace_utils.c
>
> diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
> index f8855e09b..abb221cf3 100644
> --- a/lib/librte_eal/common/eal_common_trace.c
> +++ b/lib/librte_eal/common/eal_common_trace.c
> @@ -22,6 +22,60 @@ RTE_DEFINE_PER_LCORE(int, ctf_count);
>   static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
>   static struct trace trace;
>   
> +struct trace*
> +trace_obj_get(void)
> +{
> +	return &trace;
> +}
> +
> +struct trace_point_head *
> +trace_list_head_get(void)
> +{
> +	return &tp_list;
> +}
> +
> +int
> +eal_trace_init(void)
> +{
> +	/* One of the Trace registration failed */
> +	if (trace.register_errno) {
> +		rte_errno = trace.register_errno;
> +		goto fail;
> +	}
> +
> +	if (rte_trace_global_is_disabled())
> +		return 0;
> +
> +	rte_spinlock_init(&trace.lock);
> +
> +	/* Is duplicate trace name registered */
> +	if (trace_has_duplicate_entry())
> +		goto fail;
> +
> +	/* Generate UUID ver 4 with total size of events and number of events */
> +	trace_uuid_generate();
> +
> +	/* Create trace directory */
> +	if (trace_mkdir())
> +		goto fail;
> +
> +
> +	rte_trace_global_mode_set(trace.mode);
> +
> +	return 0;
> +
> +fail:
> +	trace_err("failed to initialize trace [%s]", rte_strerror(rte_errno));
> +	return -rte_errno;
> +}
> +
> +void
> +eal_trace_fini(void)
> +{
> +	if (rte_trace_global_is_disabled())
> +		return;
> +}
> +
>   bool
>   rte_trace_global_is_enabled(void)
>   {
> diff --git a/lib/librte_eal/common/eal_common_trace_utils.c b/lib/librte_eal/common/eal_common_trace_utils.c
> new file mode 100644
> index 000000000..f7d59774c
> --- /dev/null
> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
> @@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#include <fnmatch.h>
> +#include <pwd.h>
> +#include <sys/stat.h>
> +#include <time.h>
> +
> +#include <rte_common.h>
> +#include <rte_errno.h>
> +#include <rte_string_fns.h>
> +
> +#include "eal_filesystem.h"
> +#include "eal_trace.h"
> +
> +static bool
> +trace_entry_compare(const char *name)
> +{
> +	struct trace_point_head *tp_list = trace_list_head_get();
> +	struct trace_point *tp;
> +	int count = 0;
> +
> +	STAILQ_FOREACH(tp, tp_list, next) {
> +		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
> +			count++;
> +		if (count > 1) {
> +			trace_err("found duplicate entry %s", name);
> +			rte_errno = EEXIST;
> +			return 1;

Maybe an assertion would be better here, especially since the caller 
doesn't seem to care about the fact the list is inconsistent.

Change 1 -> true.

> +		}
> +	}
> +	return 0;
return false;
> +}
> +
> +bool
> +trace_has_duplicate_entry(void)
> +{
> +	struct trace_point_head *tp_list = trace_list_head_get();
> +	struct trace_point *tp;
> +
> +	/* Is duplicate trace name registered */
> +	STAILQ_FOREACH(tp, tp_list, next)
> +		if (trace_entry_compare(tp->name))
> +			return true;
> +
> +	return false;
> +}
> +
> +void
> +trace_uuid_generate(void)
> +{
> +	struct trace_point_head *tp_list = trace_list_head_get();
> +	struct trace *trace = trace_obj_get();
> +	struct trace_point *tp;
> +	uint64_t sz_total = 0;
> +
> +	/* Go over the registered trace points to get total size of events */
> +	STAILQ_FOREACH(tp, tp_list, next) {
> +		const uint16_t sz = *tp->handle & __RTE_TRACE_FIELD_SIZE_MASK;
> +		sz_total += sz;
> +	}
> +
> +	rte_uuid_t uuid = RTE_UUID_INIT(sz_total, trace->nb_trace_points,
> +		0x4370, 0x8f50, 0x222ddd514176ULL);
> +	rte_uuid_copy(trace->uuid, uuid);
> +}
> +
> +static int
> +trace_session_name_generate(char *trace_dir)
> +{
> +	struct tm *tm_result;
> +	time_t tm;
> +	int rc;
> +
> +	tm = time(NULL);
> +	if ((int)tm == -1)
> +		goto fail;
> +
> +	tm_result = localtime(&tm);
> +	if (tm_result == NULL)
> +		goto fail;
> +
> +	rc = rte_strscpy(trace_dir,
> +			 eal_get_hugefile_prefix(), TRACE_PREFIX_LEN);
> +	if (rc == -E2BIG)
> +		rc = TRACE_PREFIX_LEN;
> +	trace_dir[rc++] = '-';
> +
> +	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
> +			"%Y-%m-%d-%p-%I-%M-%S", tm_result);
> +	if (rc == 0)
> +		goto fail;
> +
> +	return rc;
> +fail:
> +	rte_errno = errno;
> +	return -rte_errno;
> +}
> +
> +static int
> +trace_dir_default_path_get(char *dir_path)
> +{
> +	struct trace *trace = trace_obj_get();
> +	uint32_t size = sizeof(trace->dir);
> +	struct passwd *pwd;
> +	char *home_dir;
> +
> +	/* First check for shell environment variable */
> +	home_dir = getenv("HOME");
In case of the application being started with 'sudo', $HOME will not be 
"/root", but rather the original user. This sounds like a bug to me.
> +	if (home_dir == NULL) {
> +		/* Fallback to password file entry */
> +		pwd = getpwuid(getuid());
> +		if (pwd == NULL)
> +			return -EINVAL;
> +
> +		home_dir = pwd->pw_dir;
> +	}
> +
> +	/* Append dpdk-traces to directory */
> +	if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
> +		return -ENAMETOOLONG;
> +
> +	return 0;
> +}
> +
> +int
> +trace_mkdir(void)
> +{
> +	struct trace *trace = trace_obj_get();
> +	char session[TRACE_DIR_STR_LEN];
> +	char *dir_path;
> +	int rc;
> +
> +	if (!trace->dir_offset) {
> +		dir_path = (char *)calloc(1, sizeof(trace->dir));
calloc() returns a void pointer, so no need to cast it.
> +		if (dir_path == NULL) {
> +			trace_err("fail to allocate memory\n");
> +			return -ENOMEM;
> +		}
> +
> +		rc = trace_dir_default_path_get(dir_path);
> +		if (rc < 0) {
> +			trace_err("fail to get default path\n");
> +			free(dir_path);
> +			return rc;
> +		}
> +
> +	}
> +
> +	/* Create the path if it t exist, no "mkdir -p" available here */
> +	rc = mkdir(trace->dir, 0700);
> +	if (rc < 0 && errno != EEXIST) {
> +		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> +		rte_errno = errno;
> +		return -rte_errno;
> +	}
> +
> +	rc = trace_session_name_generate(session);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = mkdir(trace->dir, 0700);
> +	if (rc < 0) {
> +		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> +		rte_errno = errno;
> +		return -rte_errno;
> +	}
> +
> +	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
> +	return 0;

Is dir_path leaked? I can't find a free(), except in one of the error cases.

> +}
> +
> diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
> index 9f90a5d17..10c2f03ac 100644
> --- a/lib/librte_eal/common/eal_trace.h
> +++ b/lib/librte_eal/common/eal_trace.h
> @@ -5,7 +5,9 @@
>   #ifndef __EAL_TRACE_H
>   #define __EAL_TRACE_H
>   
> +#include <rte_spinlock.h>
>   #include <rte_trace.h>
> +#include <rte_uuid.h>
>   
>   #define trace_err(fmt, args...)\
>   	RTE_LOG(ERR, EAL, "%s():%u " fmt "\n",\
> @@ -15,6 +17,8 @@
>   	RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n",\
>   		__func__, __LINE__, ## args)
>   
> +#define TRACE_PREFIX_LEN 12
> +#define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
>   #define TRACE_CTF_FIELD_SIZE 384
>   #define TRACE_POINT_NAME_SIZE 64
>   
> @@ -26,11 +30,15 @@ struct trace_point {
>   };
>   
>   struct trace {
> +	char dir[PATH_MAX];
> +	int dir_offset;
>   	int register_errno;
>   	bool global_status;
>   	enum rte_trace_mode_e mode;
> +	rte_uuid_t uuid;
>   	uint32_t level;
>   	uint32_t nb_trace_points;
> +	rte_spinlock_t lock;
>   };
>   
>   /* Helper functions */
> @@ -41,7 +49,20 @@ trace_id_get(rte_trace_t trace)
>   		__RTE_TRACE_FIELD_ID_SHIFT;
>   }
>   
> +/* Trace object functions */
> +struct trace *trace_obj_get(void);
> +
>   /* Trace point list functions */
>   STAILQ_HEAD(trace_point_head, trace_point);
> +struct trace_point_head *trace_list_head_get(void);
> +
> +/* Util functions */
> +bool trace_has_duplicate_entry(void);
> +void trace_uuid_generate(void);
> +int trace_mkdir(void);
> +
> +/* EAL interface */
> +int eal_trace_init(void);
> +void eal_trace_fini(void);
>   
>   #endif /* __EAL_TRACE_H */
> diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build
> index 88c14ebe5..716a255d2 100644
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -29,6 +29,7 @@ common_sources = files(
>   	'eal_common_thread.c',
>   	'eal_common_timer.c',
>   	'eal_common_trace.c',
> +	'eal_common_trace_utils.c',
>   	'eal_common_uuid.c',
>   	'hotplug_mp.c',
>   	'malloc_elem.c',
> diff --git a/lib/librte_eal/freebsd/eal/Makefile b/lib/librte_eal/freebsd/eal/Makefile
> index b2fcc4212..8c444da02 100644
> --- a/lib/librte_eal/freebsd/eal/Makefile
> +++ b/lib/librte_eal/freebsd/eal/Makefile
> @@ -61,6 +61,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_proc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_fbarray.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_uuid.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_trace.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_common_trace_utils.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_malloc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += hotplug_mp.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += malloc_elem.c
> diff --git a/lib/librte_eal/linux/eal/Makefile b/lib/librte_eal/linux/eal/Makefile
> index 95470d3bb..bd9993e58 100644
> --- a/lib/librte_eal/linux/eal/Makefile
> +++ b/lib/librte_eal/linux/eal/Makefile
> @@ -69,6 +69,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_proc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_fbarray.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_uuid.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_trace.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_common_trace_utils.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_malloc.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += hotplug_mp.c
>   SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += malloc_elem.c