Re: [dpdk-dev] [PATCH] telemetry: fix error and warning printfs

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, May 22, 2020 at 7:21 PM Ciara Power <ciara.power@intel.com> wrote:
>
> Initially, printf was used to indicate and error/warning resulting from
> telemetry initialisation. This is now fixed to use EAL logs for
> warnings, and the unnecessary printf for an error is removed.
>
> Fixes: eeb486f3ba65 ("eal: add telemetry as dependency")
> Fixes: dd6275a424ac ("telemetry: fix error log output")
> Cc: david.marchand@redhat.com
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>  lib/librte_eal/freebsd/eal.c         | 4 +++-
>  lib/librte_eal/linux/eal.c           | 4 +++-
>  lib/librte_telemetry/rte_telemetry.h | 9 +++++++++
>  lib/librte_telemetry/telemetry.c     | 2 --
>  4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
> index 14b52168e..07456059e 100644
> --- a/lib/librte_eal/freebsd/eal.c
> +++ b/lib/librte_eal/freebsd/eal.c
> @@ -956,13 +956,15 @@ rte_eal_init(int argc, char **argv)
>                 return -1;
>         }
>         if (!internal_config.no_telemetry) {
> -               const char *error_str;
> +               const char *error_str = NULL;
>                 if (rte_telemetry_init(rte_eal_get_runtime_dir(),
>                                 &internal_config.ctrl_cpuset, &error_str)
>                                 != 0) {
>                         rte_eal_init_alert(error_str);
>                         return -1;
>                 }
> +               if (error_str != NULL)
> +                       RTE_LOG(WARNING, EAL, "%s\n", error_str);
>         }
>
>         eal_mcfg_complete();
> diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
> index 9620d2544..10317a660 100644
> --- a/lib/librte_eal/linux/eal.c
> +++ b/lib/librte_eal/linux/eal.c
> @@ -1294,13 +1294,15 @@ rte_eal_init(int argc, char **argv)
>                 return -1;
>         }
>         if (!internal_config.no_telemetry) {
> -               const char *error_str;
> +               const char *error_str = NULL;
>                 if (rte_telemetry_init(rte_eal_get_runtime_dir(),
>                                 &internal_config.ctrl_cpuset, &error_str)
>                                 != 0) {
>                         rte_eal_init_alert(error_str);
>                         return -1;
>                 }
> +               if (error_str != NULL)
> +                       RTE_LOG(WARNING, EAL, "%s\n", error_str);

"EAL: No legacy callbacks, legacy socket not created" comes all the times now.
Should we treat this as a warning? as it comes for with for the
built-in applications?
If it is important why not have a default handler in the EAL library?


>         }
>
>         eal_mcfg_complete();
> diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
> index 2c3c96cf7..eb7f2c917 100644
> --- a/lib/librte_telemetry/rte_telemetry.h
> +++ b/lib/librte_telemetry/rte_telemetry.h
> @@ -241,8 +241,17 @@ int
>  rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
>
>  /**
> + * @internal
>   * Initialize Telemetry.
>   *
> + * @param runtime_dir
> + * The runtime directory of DPDK.
> + * @param cpuset
> + * The CPU set to be used for setting the thread affinity.
> + * @param err_str
> + * This err_str pointer should point to NULL on entry. In the case of an error
> + * or warning, it will be non-NULL on exit.
> + *
>   * @return
>   *  0 on success.
>   * @return
> diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c
> index 7b6f8a79e..e7e3d861d 100644
> --- a/lib/librte_telemetry/telemetry.c
> +++ b/lib/librte_telemetry/telemetry.c
> @@ -403,12 +403,10 @@ rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
>  {
>         if (telemetry_v2_init(runtime_dir, cpuset) != 0) {
>                 *err_str = telemetry_log_error;
> -               printf("Error initialising telemetry - %s\n", *err_str);
>                 return -1;
>         }
>         if (telemetry_legacy_init(runtime_dir, cpuset) != 0) {
>                 *err_str = telemetry_log_error;
> -               printf("No telemetry legacy support - %s\n", *err_str);
>         }
>         return 0;
>  }
> --
> 2.17.1
>