[PATCH v5 02/23] net/ntnic: add logging implementation

[Ferruh Yigit]

On 6/27/2024 8:38 AM, Serhii Iliushyk wrote:
> Add ntnic specific implementation for logging
> 
> Signed-off-by: Serhii Iliushyk <sil-plv at napatech.com>
> ---
>  drivers/net/ntnic/meson.build           |  2 +
>  drivers/net/ntnic/ntlog/include/ntlog.h | 49 +++++++++++++++++++++++
>  drivers/net/ntnic/ntlog/ntlog.c         | 53 +++++++++++++++++++++++++
>  drivers/net/ntnic/ntnic_ethdev.c        |  2 +
>  4 files changed, 106 insertions(+)
>  create mode 100644 drivers/net/ntnic/ntlog/include/ntlog.h
>  create mode 100644 drivers/net/ntnic/ntlog/ntlog.c
> 
> diff --git a/drivers/net/ntnic/meson.build b/drivers/net/ntnic/meson.build
> index 227949eacb..b1ba8a860f 100644
> --- a/drivers/net/ntnic/meson.build
> +++ b/drivers/net/ntnic/meson.build
> @@ -15,10 +15,12 @@ cflags += [
>  # includes
>  includes = [
>      include_directories('.'),
> +    include_directories('ntlog/include'),
>

Why have 'include' folder under 'ntlog', but not put log related headers
directly under 'ntlog' folder?

>  ]
>  
>  # all sources
>  sources = files(
> +    'ntlog/ntlog.c',
>      'ntnic_ethdev.c',
>  )
>  # END
> diff --git a/drivers/net/ntnic/ntlog/include/ntlog.h b/drivers/net/ntnic/ntlog/include/ntlog.h
> new file mode 100644
> index 0000000000..58dcce0580
> --- /dev/null
> +++ b/drivers/net/ntnic/ntlog/include/ntlog.h
> @@ -0,0 +1,49 @@
> +/*
> + * SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Napatech A/S
> + */
> +
> +#ifndef NTOSS_SYSTEM_NTLOG_H
> +#define NTOSS_SYSTEM_NTLOG_H
> +
> +#include <stdarg.h>
> +#include <stdint.h>
> +#include <rte_log.h>
> +
> +extern int nt_logtype;
> +
> +#define NT_DRIVER_NAME "ntnic"
> +
> +#define NT_PMD_DRV_LOG(level, ...) \
> +	rte_log(RTE_LOG_ ## level, nt_logtype, \
> +		RTE_FMT(NT_DRIVER_NAME ": " \
> +			RTE_FMT_HEAD(__VA_ARGS__, ""), \
> +		RTE_FMT_TAIL(__VA_ARGS__, "")))
> +
> +
> +#define NT_LOG_ERR(...) NT_PMD_DRV_LOG(ERR, __VA_ARGS__)
> +#define NT_LOG_WRN(...) NT_PMD_DRV_LOG(WARNING, __VA_ARGS__)
> +#define NT_LOG_INF(...) NT_PMD_DRV_LOG(INFO, __VA_ARGS__)
> +#define NT_LOG_DBG(...) NT_PMD_DRV_LOG(DEBUG, __VA_ARGS__)
> +
> +#define NT_LOG(level, module, ...) \
> +	NT_LOG_##level(#module ": " #level ":" __VA_ARGS__)
> +
> +#define NT_LOG_DBGX(level, module, ...) \
> +		rte_log(RTE_LOG_ ##level, nt_logtype, \
> +				RTE_FMT(NT_DRIVER_NAME #module ": [%s:%u]" \
> +					RTE_FMT_HEAD(__VA_ARGS__, ""), __func__, __LINE__, \
> +				RTE_FMT_TAIL(__VA_ARGS__, "")))
> +/*
> + * nt log helper functions
> + * to create a string for NT_LOG usage to output a one-liner log
> + * to use when one single function call to NT_LOG is not optimal - that is
> + * you do not know the number of parameters at programming time or it is variable
> + */
> +char *ntlog_helper_str_alloc(const char *sinit);
> +
> +void ntlog_helper_str_add(char *s, const char *format, ...);
> +
> +void ntlog_helper_str_free(char *s);
> +
> +#endif	/* NTOSS_SYSTEM_NTLOG_H */
> diff --git a/drivers/net/ntnic/ntlog/ntlog.c b/drivers/net/ntnic/ntlog/ntlog.c
> new file mode 100644
> index 0000000000..2732a9e857
> --- /dev/null
> +++ b/drivers/net/ntnic/ntlog/ntlog.c
> @@ -0,0 +1,53 @@
> +/*
> + * SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Napatech A/S
> + */
> +
> +#include "ntlog.h"
> +
> +#include <stdarg.h>
> +#include <stddef.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +
> +#include <rte_log.h>
> +#include <rte_string_fns.h>
> +
> +#define NTLOG_HELPER_STR_SIZE_MAX (1024)
> +
> +RTE_LOG_REGISTER_DEFAULT(nt_logtype, INFO)
> +
>

Mostly default log type is 'NOTICE' to reduce the noise. Unless there is
justification, can you please set default log to notice for consistency.

> +char *ntlog_helper_str_alloc(const char *sinit)
> +{
> +	char *s = malloc(NTLOG_HELPER_STR_SIZE_MAX);
> +
> +	if (!s)
> +		return NULL;
> +
> +	if (sinit)
> +		snprintf(s, NTLOG_HELPER_STR_SIZE_MAX, "%s", sinit);
> +
>

Should it put a space after 'sinit'?

One sample of usage is like:
 ntlog_helper_str_alloc("Register::read");
 ntlog_helper_str_add(tmp_string, "(Dev: %s, ....

Won't this cause an output like:
"Register::read(Dev: ..."

Is this what intended, or is "Register::read (Dev: ..." intended?

<...>