diff mbox series

[v2,2/3] cfgfile: use RTE_LOG for errors

Message ID 20190718004822.25431-3-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Headers
Series cfgfile: cleanup patches |

Checks

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

Commit Message

Stephen Hemminger July 18, 2019, 12:48 a.m. UTC 
  In general, DPDK libraries to not print error messages to
stdout because that is often redirected to /dev/null for daemons.
This patch changes cfgfile library to use RTE_LOG with its
own type.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/Makefile      |  1 +
 lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 10 deletions(-)

Comments

Bruce Richardson July 18, 2019, 8:31 a.m. UTC | #1 
On Wed, Jul 17, 2019 at 05:48:21PM -0700, Stephen Hemminger wrote:
> In general, DPDK libraries to not print error messages to
> stdout because that is often redirected to /dev/null for daemons.
> This patch changes cfgfile library to use RTE_LOG with its
> own type.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_cfgfile/Makefile      |  1 +
>  lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_cfgfile/Makefile b/lib/librte_cfgfile/Makefile
> index d9512565e559..4fc711778699 100644
> --- a/lib/librte_cfgfile/Makefile
> +++ b/lib/librte_cfgfile/Makefile
> @@ -11,6 +11,7 @@ LIB = librte_cfgfile.a
>  CFLAGS += -O3
>  CFLAGS += $(WERROR_FLAGS)
>  CFLAGS += -I$(SRCDIR)/../librte_eal/common/include
> +LDFLAGS += -lrte_log
>  
Where does this come from, there is no separate log library in DPDK?
Stephen Hemminger July 18, 2019, 2:34 p.m. UTC | #2 
On Thu, 18 Jul 2019 09:31:11 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Wed, Jul 17, 2019 at 05:48:21PM -0700, Stephen Hemminger wrote:
> > In general, DPDK libraries to not print error messages to
> > stdout because that is often redirected to /dev/null for daemons.
> > This patch changes cfgfile library to use RTE_LOG with its
> > own type.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_cfgfile/Makefile      |  1 +
> >  lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
> >  2 files changed, 29 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/librte_cfgfile/Makefile b/lib/librte_cfgfile/Makefile
> > index d9512565e559..4fc711778699 100644
> > --- a/lib/librte_cfgfile/Makefile
> > +++ b/lib/librte_cfgfile/Makefile
> > @@ -11,6 +11,7 @@ LIB = librte_cfgfile.a
> >  CFLAGS += -O3
> >  CFLAGS += $(WERROR_FLAGS)
> >  CFLAGS += -I$(SRCDIR)/../librte_eal/common/include
> > +LDFLAGS += -lrte_log
> >    
> Where does this come from, there is no separate log library in DPDK?

I saw a build failure with previous patch about rte_log not being
present.
Bruce Richardson July 18, 2019, 2:36 p.m. UTC | #3 
On Thu, Jul 18, 2019 at 07:34:59AM -0700, Stephen Hemminger wrote:
> On Thu, 18 Jul 2019 09:31:11 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Wed, Jul 17, 2019 at 05:48:21PM -0700, Stephen Hemminger wrote:
> > > In general, DPDK libraries to not print error messages to
> > > stdout because that is often redirected to /dev/null for daemons.
> > > This patch changes cfgfile library to use RTE_LOG with its
> > > own type.
> > > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > >  lib/librte_cfgfile/Makefile      |  1 +
> > >  lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
> > >  2 files changed, 29 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/lib/librte_cfgfile/Makefile b/lib/librte_cfgfile/Makefile
> > > index d9512565e559..4fc711778699 100644
> > > --- a/lib/librte_cfgfile/Makefile
> > > +++ b/lib/librte_cfgfile/Makefile
> > > @@ -11,6 +11,7 @@ LIB = librte_cfgfile.a
> > >  CFLAGS += -O3
> > >  CFLAGS += $(WERROR_FLAGS)
> > >  CFLAGS += -I$(SRCDIR)/../librte_eal/common/include
> > > +LDFLAGS += -lrte_log
> > >    
> > Where does this come from, there is no separate log library in DPDK?
> 
> I saw a build failure with previous patch about rte_log not being
> present.

I can believe that, it's just how does this help? What does the linker pick
up when you pass this?
Stephen Hemminger July 18, 2019, 2:41 p.m. UTC | #4 
On Thu, 18 Jul 2019 15:36:45 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Jul 18, 2019 at 07:34:59AM -0700, Stephen Hemminger wrote:
> > On Thu, 18 Jul 2019 09:31:11 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >   
> > > On Wed, Jul 17, 2019 at 05:48:21PM -0700, Stephen Hemminger wrote:  
> > > > In general, DPDK libraries to not print error messages to
> > > > stdout because that is often redirected to /dev/null for daemons.
> > > > This patch changes cfgfile library to use RTE_LOG with its
> > > > own type.
> > > > 
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > >  lib/librte_cfgfile/Makefile      |  1 +
> > > >  lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
> > > >  2 files changed, 29 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/lib/librte_cfgfile/Makefile b/lib/librte_cfgfile/Makefile
> > > > index d9512565e559..4fc711778699 100644
> > > > --- a/lib/librte_cfgfile/Makefile
> > > > +++ b/lib/librte_cfgfile/Makefile
> > > > @@ -11,6 +11,7 @@ LIB = librte_cfgfile.a
> > > >  CFLAGS += -O3
> > > >  CFLAGS += $(WERROR_FLAGS)
> > > >  CFLAGS += -I$(SRCDIR)/../librte_eal/common/include
> > > > +LDFLAGS += -lrte_log
> > > >      
> > > Where does this come from, there is no separate log library in DPDK?  
> > 
> > I saw a build failure with previous patch about rte_log not being
> > present.  
> 
> I can believe that, it's just how does this help? What does the linker pick
> up when you pass this?

I don't think it helps, it was an ICC only error and it was purely
a guess since I don't have ICC.

Let me take it off in the next version and see what CI says.
Stephen Hemminger July 18, 2019, 5:12 p.m. UTC | #5 
On Thu, 18 Jul 2019 15:36:45 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Jul 18, 2019 at 07:34:59AM -0700, Stephen Hemminger wrote:
> > On Thu, 18 Jul 2019 09:31:11 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >   
> > > On Wed, Jul 17, 2019 at 05:48:21PM -0700, Stephen Hemminger wrote:  
> > > > In general, DPDK libraries to not print error messages to
> > > > stdout because that is often redirected to /dev/null for daemons.
> > > > This patch changes cfgfile library to use RTE_LOG with its
> > > > own type.
> > > > 
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > >  lib/librte_cfgfile/Makefile      |  1 +
> > > >  lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
> > > >  2 files changed, 29 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/lib/librte_cfgfile/Makefile b/lib/librte_cfgfile/Makefile
> > > > index d9512565e559..4fc711778699 100644
> > > > --- a/lib/librte_cfgfile/Makefile
> > > > +++ b/lib/librte_cfgfile/Makefile
> > > > @@ -11,6 +11,7 @@ LIB = librte_cfgfile.a
> > > >  CFLAGS += -O3
> > > >  CFLAGS += $(WERROR_FLAGS)
> > > >  CFLAGS += -I$(SRCDIR)/../librte_eal/common/include
> > > > +LDFLAGS += -lrte_log
> > > >      
> > > Where does this come from, there is no separate log library in DPDK?  
> > 
> > I saw a build failure with previous patch about rte_log not being
> > present.  
> 
> I can believe that, it's just how does this help? What does the linker pick
> up when you pass this?


Not sure what causes this:

*Make Build Failed #1:
OS: RHEL80-64
Target: x86_64-native-linuxapp-gcc+shared
rte_cfgfile.o:rte_cfgfile.c:(.text.unlikely+0xbe): more undefined references to `rte_log' follow
rte_cfgfile.o: In function `cfgfile_init':
rte_cfgfile.c:(.text.startup+0xc): undefined reference to `rte_log_register'
rte_cfgfile.c:(.text.startup+0x25): undefined reference to `rte_log_set_level'
collect2: error: ld returned 1 exit status
make[5]: *** [/tmp/RHEL80-64_K3.10.0_GCC8.2.1/x86_64-native-linuxapp-gcc+shared/6baa0548e7644f418e02f19043f86f82/dpdk/mk/rte.lib.mk:102: librte_cfgfile.so.2.1] Error 1
make[4]: *** [/tmp/RHEL80-64_K3.10.0_GCC8.2.1/x86_64-native-linuxapp-gcc+shared/6baa0548e7644f418e02f19043f86f82/dpdk/mk/rte.subdir.mk:37: librte_cfgfile] Error 2
make[4]: *** Waiting for unfinished jobs....
== Build lib/librte_eal/common
  SYMLINK-FILE include/rte_common.h
  SYMLINK-FILE include/rte_compat.h
  SYMLINK-FILE include/rte_branch_prediction.h
--
diff mbox series

Patch

diff --git a/lib/librte_cfgfile/Makefile b/lib/librte_cfgfile/Makefile
index d9512565e559..4fc711778699 100644
--- a/lib/librte_cfgfile/Makefile
+++ b/lib/librte_cfgfile/Makefile
@@ -11,6 +11,7 @@  LIB = librte_cfgfile.a
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 CFLAGS += -I$(SRCDIR)/../librte_eal/common/include
+LDFLAGS += -lrte_log
 
 EXPORT_MAP := rte_cfgfile_version.map
 
diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 1ef298592fa5..388415147930 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -9,6 +9,7 @@ 
 #include <errno.h>
 #include <rte_string_fns.h>
 #include <rte_common.h>
+#include <rte_log.h>
 
 #include "rte_cfgfile.h"
 
@@ -26,6 +27,12 @@  struct rte_cfgfile {
 	struct rte_cfgfile_section *sections;
 };
 
+static int cfgfile_logtype;
+
+#define CFG_LOG(level, fmt, args...)					\
+	rte_log(RTE_LOG_ ## level, cfgfile_logtype, "%s(): " fmt "\n",	\
+		__func__, ## args)
+
 /** when we resize a file structure, how many extra entries
  * for new sections do we add in */
 #define CFG_ALLOC_SECTION_BATCH 8
@@ -128,7 +135,7 @@  rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
 	unsigned int i;
 
 	if (!params) {
-		printf("Error - missing cfgfile parameters\n");
+		CFG_LOG(ERR, "missing cfgfile parameters\n");
 		return -EINVAL;
 	}
 
@@ -141,7 +148,7 @@  rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
 	}
 
 	if (valid_comment == 0)	{
-		printf("Error - invalid comment characters %c\n",
+		CFG_LOG(ERR, "invalid comment characters %c\n",
 		       params->comment_character);
 		return -ENOTSUP;
 	}
@@ -178,7 +185,7 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 		size_t len = strnlen(buffer, sizeof(buffer));
 		lineno++;
 		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
-			printf("Error line %d - no \\n found on string. "
+			CFG_LOG(ERR, " line %d - no \\n found on string. "
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
@@ -198,8 +205,9 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 			/* section heading line */
 			char *end = memchr(buffer, ']', len);
 			if (end == NULL) {
-				printf("Error line %d - no terminating ']'"
-					"character found\n", lineno);
+				CFG_LOG(ERR,
+					"line %d - no terminating ']' character found\n",
+					lineno);
 				goto error1;
 			}
 			*end = '\0';
@@ -213,8 +221,9 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 			split[0] = buffer;
 			split[1] = memchr(buffer, '=', len);
 			if (split[1] == NULL) {
-				printf("Error line %d - no '='"
-					"character found\n", lineno);
+				CFG_LOG(ERR,
+					"line %d - no '=' character found\n",
+					lineno);
 				goto error1;
 			}
 			*split[1] = '\0';
@@ -236,8 +245,9 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 
 			if (!(flags & CFG_FLAG_EMPTY_VALUES) &&
 					(*split[1] == '\0')) {
-				printf("Error at line %d - cannot use empty "
-							"values\n", lineno);
+				CFG_LOG(ERR,
+					"line %d - cannot use empty values\n",
+					lineno);
 				goto error1;
 			}
 
@@ -397,7 +407,8 @@  int rte_cfgfile_set_entry(struct rte_cfgfile *cfg, const char *sectionname,
 				sizeof(curr_section->entries[i].value));
 			return 0;
 		}
-	printf("Error - entry name doesn't exist\n");
+
+	CFG_LOG(ERR, "entry name doesn't exist\n");
 	return -EINVAL;
 }
 
@@ -552,3 +563,10 @@  rte_cfgfile_has_entry(struct rte_cfgfile *cfg, const char *sectionname,
 {
 	return rte_cfgfile_get_entry(cfg, sectionname, entryname) != NULL;
 }
+
+RTE_INIT(cfgfile_init)
+{
+	cfgfile_logtype = rte_log_register("lib.cfgfile");
+	if (cfgfile_logtype >= 0)
+		rte_log_set_level(cfgfile_logtype, RTE_LOG_INFO);
+}