telemetry: fix error and warning printfs
diff mbox series

Message ID 20200522134839.15911-1-ciara.power@intel.com
State Accepted, archived
Headers show
Series
  • telemetry: fix error and warning printfs
Related show

Checks

Context Check Description
ci/Intel-compilation fail apply issues
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Ciara Power May 22, 2020, 1:48 p.m. UTC
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(-)

Comments

Bruce Richardson May 22, 2020, 3 p.m. UTC | #1
On Fri, May 22, 2020 at 02:48:39PM +0100, Ciara Power 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>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
Jerin Jacob May 22, 2020, 4:34 p.m. UTC | #2
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
>
Bruce Richardson May 22, 2020, 4:48 p.m. UTC | #3
On Fri, May 22, 2020 at 10:04:44PM +0530, Jerin Jacob wrote:
> 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?
> 
Perhaps the warning level could be changed to INFO.
BTW: if you compile with jansson library available, the legacy callbacks
will be available and this will be silenced.

/Bruce
Jerin Jacob May 22, 2020, 5 p.m. UTC | #4
On Fri, May 22, 2020 at 10:18 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, May 22, 2020 at 10:04:44PM +0530, Jerin Jacob wrote:
> > 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?
> >
> Perhaps the warning level could be changed to INFO.

OK.

> BTW: if you compile with jansson library available, the legacy callbacks
> will be available and this will be silenced.

I see.

>
> /Bruce
Thomas Monjalon May 24, 2020, 4:05 p.m. UTC | #5
22/05/2020 18:48, Bruce Richardson:
> On Fri, May 22, 2020 at 10:04:44PM +0530, Jerin Jacob wrote:
> > 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>
> > > ---
> > > --- a/lib/librte_eal/linux/eal.c
> > > +++ b/lib/librte_eal/linux/eal.c
> > > +               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?
> 
> Perhaps the warning level could be changed to INFO.
> BTW: if you compile with jansson library available, the legacy callbacks
> will be available and this will be silenced.

Applied with log level set as NOTICE.
Bruce Richardson May 25, 2020, 9:55 a.m. UTC | #6
On Sun, May 24, 2020 at 06:05:53PM +0200, Thomas Monjalon wrote:
> 22/05/2020 18:48, Bruce Richardson:
> > On Fri, May 22, 2020 at 10:04:44PM +0530, Jerin Jacob wrote:
> > > 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>
> > > > ---
> > > > --- a/lib/librte_eal/linux/eal.c
> > > > +++ b/lib/librte_eal/linux/eal.c
> > > > +               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?
> > 
> > Perhaps the warning level could be changed to INFO.
> > BTW: if you compile with jansson library available, the legacy callbacks
> > will be available and this will be silenced.
> 
> Applied with log level set as NOTICE.
> 
Thanks.

Patch
diff mbox series

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_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;
 }