[v2,2/3] cfgfile: use RTE_LOG for errors
Checks
Commit Message
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
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?
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.
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?
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.
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
--
@@ -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
@@ -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);
+}