[dpdk-dev] [PATCH 0/3] cfgfile: cleanup patches

[dpdk-dev] [PATCH 0/3] cfgfile: cleanup patches

[Stephen Hemminger]

Some small patches for the cfgfile library

Stephen Hemminger (3):
  cfgfile: remove unnecessary initialization
  cfgfile: use RTE_LOG for errors
  cfgfile: use calloc

 lib/librte_cfgfile/rte_cfgfile.c        | 42 ++++++++++++++-----------
 lib/librte_eal/common/include/rte_log.h |  1 +
 2 files changed, 24 insertions(+), 19 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/3] cfgfile: remove unnecessary initialization

[Stephen Hemminger]

No need to initialize variable if it is immediately overwritten.
It is better style not do unnecessary initialization with modern
tools since it lets compiler and other static checkers detect
uninitialized data.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index f8e7627a5169..1ef298592fa5 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -160,9 +160,9 @@ struct rte_cfgfile *
 rte_cfgfile_load_with_params(const char *filename, int flags,
 			     const struct rte_cfgfile_parameters *params)
 {
-	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4];
 	int lineno = 0;
-	struct rte_cfgfile *cfg = NULL;
+	struct rte_cfgfile *cfg;
 
 	if (rte_cfgfile_check_params(params))
 		return NULL;
@@ -174,7 +174,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 	cfg = rte_cfgfile_create(flags);
 
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
-		char *pos = NULL;
+		char *pos;
 		size_t len = strnlen(buffer, sizeof(buffer));
 		lineno++;
 		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
@@ -260,7 +260,7 @@ struct rte_cfgfile *
 rte_cfgfile_create(int flags)
 {
 	int i;
-	struct rte_cfgfile *cfg = NULL;
+	struct rte_cfgfile *cfg;
 
 	cfg = malloc(sizeof(*cfg));
 
-- 
2.20.1

[dpdk-dev] [PATCH 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

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/rte_cfgfile.c        | 25 +++++++++++++++----------
 lib/librte_eal/common/include/rte_log.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 1ef298592fa5..c4b768b6833f 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"
 
@@ -128,7 +129,7 @@ rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
 	unsigned int i;
 
 	if (!params) {
-		printf("Error - missing cfgfile parameters\n");
+		RTE_LOG(ERR, CFGFILE, "missing cfgfile parameters\n");
 		return -EINVAL;
 	}
 
@@ -141,7 +142,7 @@ rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
 	}
 
 	if (valid_comment == 0)	{
-		printf("Error - invalid comment characters %c\n",
+		RTE_LOG(ERR, CFGFILE, "invalid comment characters %c\n",
 		       params->comment_character);
 		return -ENOTSUP;
 	}
@@ -178,7 +179,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. "
+			RTE_LOG(ERR, CFGFILE, " line %d - no \\n found on string. "
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
@@ -198,8 +199,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);
+				RTE_LOG(ERR, CFGFILE,
+					"line %d - no terminating ']' character found\n",
+					lineno);
 				goto error1;
 			}
 			*end = '\0';
@@ -213,8 +215,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);
+				RTE_LOG(ERR, CFGFILE,
+					"line %d - no '=' character found\n",
+					lineno);
 				goto error1;
 			}
 			*split[1] = '\0';
@@ -236,8 +239,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);
+				RTE_LOG(ERR, CFGFILE,
+					"line %d - cannot use empty values\n",
+					lineno);
 				goto error1;
 			}
 
@@ -397,7 +401,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");
+
+	RTE_LOG(ERR, CFGFILE, "entry name doesn't exist\n");
 	return -EINVAL;
 }
 
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index cbb41846aaa3..fa747d5b90ef 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -62,6 +62,7 @@ extern struct rte_logs rte_logs;
 #define RTE_LOGTYPE_EFD       18 /**< Log related to EFD. */
 #define RTE_LOGTYPE_EVENTDEV  19 /**< Log related to eventdev. */
 #define RTE_LOGTYPE_GSO       20 /**< Log related to GSO. */
+#define RTE_LOGTYPE_CFGFILE   21 /**< Log related to cfgfile. */
 
 /* these log types can be used in an application */
 #define RTE_LOGTYPE_USER1     24 /**< User-defined log type 1. */
-- 
2.20.1

[dpdk-dev] [PATCH 3/3] cfgfile: use calloc

[Stephen Hemminger]

Better to use calloc when allocating arrays.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index c4b768b6833f..39fec4b82bce 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -275,17 +275,16 @@ rte_cfgfile_create(int flags)
 	cfg->num_sections = 0;
 
 	/* allocate first batch of sections and entries */
-	cfg->sections = malloc(sizeof(struct rte_cfgfile_section) *
-			CFG_ALLOC_SECTION_BATCH);
-
+	cfg->sections = calloc(CFG_ALLOC_SECTION_BATCH,
+			       sizeof(struct rte_cfgfile_section));
 	if (cfg->sections == NULL)
 		goto error1;
 
 	cfg->allocated_sections = CFG_ALLOC_SECTION_BATCH;
 
 	for (i = 0; i < CFG_ALLOC_SECTION_BATCH; i++) {
-		cfg->sections[i].entries = malloc(sizeof(
-			struct rte_cfgfile_entry) * CFG_ALLOC_ENTRY_BATCH);
+		cfg->sections[i].entries = calloc(CFG_ALLOC_ENTRY_BATCH,
+					  sizeof(struct rte_cfgfile_entry));
 
 		if (cfg->sections[i].entries == NULL)
 			goto error1;
-- 
2.20.1

Re: [dpdk-dev] [PATCH 0/3] cfgfile: cleanup patches

[Bruce Richardson]

On Tue, Jul 16, 2019 at 10:27:38AM -0700, Stephen Hemminger wrote:
> Some small patches for the cfgfile library
> 
> Stephen Hemminger (3):
>   cfgfile: remove unnecessary initialization
>   cfgfile: use RTE_LOG for errors
>   cfgfile: use calloc
> 
Looks ok to me, and compile testing showed no issues on my system.

Series-Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH 2/3] cfgfile: use RTE_LOG for errors

[Thomas Monjalon]

16/07/2019 19:27, Stephen Hemminger:
> 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>
> ---
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -62,6 +62,7 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOGTYPE_EFD       18 /**< Log related to EFD. */
>  #define RTE_LOGTYPE_EVENTDEV  19 /**< Log related to eventdev. */
>  #define RTE_LOGTYPE_GSO       20 /**< Log related to GSO. */
> +#define RTE_LOGTYPE_CFGFILE   21 /**< Log related to cfgfile. */

As you know, we are supposed to use dynamic logging now.
Let's stop to add new static log types here.
Better, we should plan to completely drop these types.

Re: [dpdk-dev] [PATCH 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

On Wed, 17 Jul 2019 23:01:00 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 16/07/2019 19:27, Stephen Hemminger:
> > 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>
> > ---
> > --- a/lib/librte_eal/common/include/rte_log.h
> > +++ b/lib/librte_eal/common/include/rte_log.h
> > @@ -62,6 +62,7 @@ extern struct rte_logs rte_logs;
> >  #define RTE_LOGTYPE_EFD       18 /**< Log related to EFD. */
> >  #define RTE_LOGTYPE_EVENTDEV  19 /**< Log related to eventdev. */
> >  #define RTE_LOGTYPE_GSO       20 /**< Log related to GSO. */
> > +#define RTE_LOGTYPE_CFGFILE   21 /**< Log related to cfgfile. */  
> 
> As you know, we are supposed to use dynamic logging now.
> Let's stop to add new static log types here.
> Better, we should plan to completely drop these types.
> 
> 

Ok, but rte_cfgfile can be used before eal init.

[dpdk-dev] [PATCH v2 0/3] cfgfile: cleanup patches

[Stephen Hemminger]

Small cleanups to cfgfile library

Stephen Hemminger (3):
  cfgfile: remove unnecessary initialization
  cfgfile: use RTE_LOG for errors
  cfgfile: use calloc

 lib/librte_cfgfile/Makefile      |  1 +
 lib/librte_cfgfile/rte_cfgfile.c | 55 +++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 19 deletions(-)

v2 - use dynamic logtype instead of introducing new static value

-- 
2.17.1

[dpdk-dev] [PATCH v2 1/3] cfgfile: remove unnecessary initialization

[Stephen Hemminger]

No need to initialize variable if it is immediately overwritten.
It is better style not do unnecessary initialization with modern
tools since it lets compiler and other static checkers detect
uninitialized data.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index f8e7627a5169..1ef298592fa5 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -160,9 +160,9 @@ struct rte_cfgfile *
 rte_cfgfile_load_with_params(const char *filename, int flags,
 			     const struct rte_cfgfile_parameters *params)
 {
-	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4];
 	int lineno = 0;
-	struct rte_cfgfile *cfg = NULL;
+	struct rte_cfgfile *cfg;
 
 	if (rte_cfgfile_check_params(params))
 		return NULL;
@@ -174,7 +174,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 	cfg = rte_cfgfile_create(flags);
 
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
-		char *pos = NULL;
+		char *pos;
 		size_t len = strnlen(buffer, sizeof(buffer));
 		lineno++;
 		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
@@ -260,7 +260,7 @@ struct rte_cfgfile *
 rte_cfgfile_create(int flags)
 {
 	int i;
-	struct rte_cfgfile *cfg = NULL;
+	struct rte_cfgfile *cfg;
 
 	cfg = malloc(sizeof(*cfg));
 
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

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
 
 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);
+}
-- 
2.17.1

[dpdk-dev] [PATCH v2 3/3] cfgfile: use calloc

[Stephen Hemminger]

Better to use calloc when allocating arrays.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 388415147930..9049fd9c2319 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -281,17 +281,16 @@ rte_cfgfile_create(int flags)
 	cfg->num_sections = 0;
 
 	/* allocate first batch of sections and entries */
-	cfg->sections = malloc(sizeof(struct rte_cfgfile_section) *
-			CFG_ALLOC_SECTION_BATCH);
-
+	cfg->sections = calloc(CFG_ALLOC_SECTION_BATCH,
+			       sizeof(struct rte_cfgfile_section));
 	if (cfg->sections == NULL)
 		goto error1;
 
 	cfg->allocated_sections = CFG_ALLOC_SECTION_BATCH;
 
 	for (i = 0; i < CFG_ALLOC_SECTION_BATCH; i++) {
-		cfg->sections[i].entries = malloc(sizeof(
-			struct rte_cfgfile_entry) * CFG_ALLOC_ENTRY_BATCH);
+		cfg->sections[i].entries = calloc(CFG_ALLOC_ENTRY_BATCH,
+					  sizeof(struct rte_cfgfile_entry));
 
 		if (cfg->sections[i].entries == NULL)
 			goto error1;
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 2/3] cfgfile: use RTE_LOG for errors

[Bruce Richardson]

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?

Re: [dpdk-dev] [PATCH v2 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH v2 2/3] cfgfile: use RTE_LOG for errors

[Bruce Richardson]

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?

Re: [dpdk-dev] [PATCH v2 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH v2 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

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
--

[dpdk-dev] [PATCH v3 0/3] cfgfile: cleanup patches

[Stephen Hemminger]

Small cleanups to cfgfile library.

Stephen Hemminger (3):
  cfgfile: remove unnecessary initialization
  cfgfile: use RTE_LOG for errors
  cfgfile: use calloc

 lib/Makefile                     |  1 +
 lib/librte_cfgfile/rte_cfgfile.c | 55 +++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 19 deletions(-)

v3 - cfgfile now depends on rte_log from eal so fix Makefile
v2 - use dynamic logtype instead of introducing new static value

-- 
2.17.1

[dpdk-dev] [PATCH v3 1/3] cfgfile: remove unnecessary initialization

[Stephen Hemminger]

No need to initialize variable if it is immediately overwritten.
It is better style not do unnecessary initialization with modern
tools since it lets compiler and other static checkers detect
uninitialized data.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index f8e7627a5169..1ef298592fa5 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -160,9 +160,9 @@ struct rte_cfgfile *
 rte_cfgfile_load_with_params(const char *filename, int flags,
 			     const struct rte_cfgfile_parameters *params)
 {
-	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4];
 	int lineno = 0;
-	struct rte_cfgfile *cfg = NULL;
+	struct rte_cfgfile *cfg;
 
 	if (rte_cfgfile_check_params(params))
 		return NULL;
@@ -174,7 +174,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 	cfg = rte_cfgfile_create(flags);
 
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
-		char *pos = NULL;
+		char *pos;
 		size_t len = strnlen(buffer, sizeof(buffer));
 		lineno++;
 		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
@@ -260,7 +260,7 @@ struct rte_cfgfile *
 rte_cfgfile_create(int flags)
 {
 	int i;
-	struct rte_cfgfile *cfg = NULL;
+	struct rte_cfgfile *cfg;
 
 	cfg = malloc(sizeof(*cfg));
 
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/3] cfgfile: use RTE_LOG for errors

[Stephen Hemminger]

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/Makefile                     |  1 +
 lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index bdb8a67e2d9d..41c463d92139 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -19,6 +19,7 @@ DEPDIRS-librte_mbuf := librte_eal librte_mempool
 DIRS-$(CONFIG_RTE_LIBRTE_TIMER) += librte_timer
 DEPDIRS-librte_timer := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
+DEPDIRS-librte_cfgfile := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
 DEPDIRS-librte_cmdline := librte_eal librte_net
 DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ethdev
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);
+}
-- 
2.17.1

[dpdk-dev] [PATCH v3 3/3] cfgfile: use calloc

[Stephen Hemminger]

Better to use calloc when allocating arrays.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 388415147930..9049fd9c2319 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -281,17 +281,16 @@ rte_cfgfile_create(int flags)
 	cfg->num_sections = 0;
 
 	/* allocate first batch of sections and entries */
-	cfg->sections = malloc(sizeof(struct rte_cfgfile_section) *
-			CFG_ALLOC_SECTION_BATCH);
-
+	cfg->sections = calloc(CFG_ALLOC_SECTION_BATCH,
+			       sizeof(struct rte_cfgfile_section));
 	if (cfg->sections == NULL)
 		goto error1;
 
 	cfg->allocated_sections = CFG_ALLOC_SECTION_BATCH;
 
 	for (i = 0; i < CFG_ALLOC_SECTION_BATCH; i++) {
-		cfg->sections[i].entries = malloc(sizeof(
-			struct rte_cfgfile_entry) * CFG_ALLOC_ENTRY_BATCH);
+		cfg->sections[i].entries = calloc(CFG_ALLOC_ENTRY_BATCH,
+					  sizeof(struct rte_cfgfile_entry));
 
 		if (cfg->sections[i].entries == NULL)
 			goto error1;
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 2/3] cfgfile: use RTE_LOG for errors

[Thomas Monjalon]

18/07/2019 19:18, Stephen Hemminger:
> 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/Makefile                     |  1 +
>  lib/librte_cfgfile/rte_cfgfile.c | 38 +++++++++++++++++++++++---------
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> --- a/lib/Makefile
> +++ b/lib/Makefile
>  DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
> +DEPDIRS-librte_cfgfile := librte_eal

You are missing LDLIBS += -lrte_eal in the librte_cfgfile Makefile.

I can add it while applying.

Re: [dpdk-dev] [PATCH v3 0/3] cfgfile: cleanup patches

[Thomas Monjalon]

18/07/2019 19:18, Stephen Hemminger:
> Small cleanups to cfgfile library.
> 
> Stephen Hemminger (3):
>   cfgfile: remove unnecessary initialization
>   cfgfile: use RTE_LOG for errors
>   cfgfile: use calloc
> 
>  lib/Makefile                     |  1 +
>  lib/librte_cfgfile/rte_cfgfile.c | 55 +++++++++++++++++++++-----------
>  2 files changed, 37 insertions(+), 19 deletions(-)
> 
> v3 - cfgfile now depends on rte_log from eal so fix Makefile
> v2 - use dynamic logtype instead of introducing new static value

Applied with a fix in Makefile, thanks