Skip to contentSkip to conversation

logging enhancments

PATCH Superseded Stephen Hemminger 3 participants 14 messages Feb 23, 2018 – Apr 23, 2018

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

cover logging enhancments

The current dynamic logging has some awkward user interface choices. It uses integers for log levels which requires user to know the mapping between numeric and symbolic values.

A bigger problem was the choice of regular expressions and option format for dynamic logging. Dynamic log names are seperated with a period and the wildcard character for regular expressions is a period. It is just a happy accident the expressions like:

	"pmd.net.virtio.*"

work as expected. This patch set adds a more usable solution with filename style matching.

Also, the choice of comma as seperator for log-level option was not consistent with other options. For other options, comma is used to seperate list of equal values as in:

	-l 1,2,3

Since new match required a backwards compatiable option the colon is now used to seperate name and value.

So:

	--log-level='pmd.net.virtio.*,7'
still works as expected. But the prefered syntax is:
	--log-level='pmd.net.virtio.*:info'

If this is accepted, I think we should mark the old regex style matching as deprecated and remove it in 18.11??

Also, the dynamic log level pattern stuff is not adaquately documented right now. There are only a couple of vague references in the current documentation (which this updates).

Stephen Hemminger (3):
eal: allow symbolic log levels
log: add ability to match dynamic log based on shell pattern
doc: update log level matching in documentation

 doc/guides/contributing/coding_style.rst   |  2 +-
 doc/guides/nics/qede.rst                   |  2 +-
 lib/librte_eal/common/eal_common_log.c     | 22 +++++++-
 lib/librte_eal/common/eal_common_options.c | 86 +++++++++++++++++++++++-------
 lib/librte_eal/common/include/rte_log.h    | 16 +++++-
 5 files changed, 103 insertions(+), 25 deletions(-)
Signature · 1 line
2.16.1
author
#1 lore raw

PATCH 1/3 eal: allow symbolic log levels

Much easeier to remember names than numbers. Allows

	--log-level=pmd.net.ixgbe.*,debug

The option argument allow shortened form so

	--log-level=pmd.net.ixgbe.*,i

also works.

Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/librte_eal/common/eal_common_options.c | 68 ++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 17 deletions(-)
lib/librte_eal/common/eal_common_options.c +51 -17
index 9f2f8d25afd9..f2339c3907e4 100644
@@ -911,10 +911,52 @@ eal_parse_syslog(const char *facility, struct internal_config *conf)
 }
 
 static int
-eal_parse_log_level(const char *arg)
+eal_parse_log_priority(const char *level)
 {
-	char *end, *str, *type, *level;
+	static const char *levels[] = {
+		[RTE_LOG_EMERG]   = "emergency",
+		[RTE_LOG_ALERT]   = "alert",
+		[RTE_LOG_CRIT]    = "critical",
+		[RTE_LOG_ERR]     = "error",
+		[RTE_LOG_WARNING] = "warning",
+		[RTE_LOG_NOTICE]  = "notice",
+		[RTE_LOG_INFO]    = "info",
+		[RTE_LOG_DEBUG]   = "debug",
+	};
+	size_t len = strlen(level);
 	unsigned long tmp;
+	char *end;
+	unsigned int i;
+
+	if (len == 0)
+		return -1;
+
+	/* look for named values, skip 0 which is not a valid level */
+	for (i = 1; i < sizeof(levels) / sizeof(levels[0]); i++) {
+		if (strncmp(levels[i], level, len) == 0)
+			return i;
+	}
+
+	/* not a string, maybe it is numeric */
+	errno = 0;
+	tmp = strtoul(level, &end, 0);
+
+	/* check for errors */
+	if (errno != 0 || end == NULL || *end != '\0')
+		return -1;
+
+	/* log_level is a uint32_t */
+	if (tmp >= UINT32_MAX)
+		return -1;
+
+	return tmp;
+}
+
+static int
+eal_parse_log_level(const char *arg)
+{
+	char *str, *type, *level;
+	int priority;
 
 	str = strdup(arg);
 	if (str == NULL)

@@ -928,23 +970,15 @@ eal_parse_log_level(const char *arg)
 		level = strsep(&str, ",");
 	}
 
-	errno = 0;
-	tmp = strtoul(level, &end, 0);
-
-	/* check for errors */
-	if ((errno != 0) || (level[0] == '\0') ||
-		    end == NULL || (*end != '\0'))
+	priority = eal_parse_log_priority(level);
+	if (priority < 0)
 		goto fail;
-
-	/* log_level is a uint32_t */
-	if (tmp >= UINT32_MAX)
-		goto fail;
-
+	
 	if (type == NULL) {
-		rte_log_set_global_level(tmp);
-	} else if (rte_log_set_level_regexp(type, tmp) < 0) {
-		printf("cannot set log level %s,%lu\n",
-			type, tmp);
+		rte_log_set_global_level(priority);
+	} else if (rte_log_set_level_regexp(type, priority) < 0) {
+		fprintf(stderr, "cannot set log level %s,%d\n",
+			type, priority);
 		goto fail;
 	}
Signature · 1 line
2.16.1
author
#2 lore raw

PATCH 2/3 log: add ability to match dynamic log based on shell pattern

In reply to Stephen Hemminger

Regular expressions are not the best way to match a hierarchal pattern like dynamic log levels. And the seperator for dynamic log levels is period which is the regex wildcard character.

A better solution is to use filename matching 'globbing' so that log levels match like file paths. For compatiablity, use colon to seperate pattern match style arguments. For exmaple:

	--log-level 'pmd.net.virtio.*:debug'
Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/librte_eal/common/eal_common_log.c     | 22 +++++++++++++++++-
 lib/librte_eal/common/eal_common_options.c | 36 ++++++++++++++++++++----------
 lib/librte_eal/common/include/rte_log.h    | 16 +++++++++++--
 3 files changed, 59 insertions(+), 15 deletions(-)
lib/librte_eal/common/eal_common_log.c +21 -1
index 37b2e20e539b..2601c7cd4e4f 100644
@@ -9,6 +9,7 @@
 #include <string.h>
 #include <errno.h>
 #include <regex.h>
+#include <fnmatch.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>

@@ -89,7 +90,26 @@ rte_log_set_level(uint32_t type, uint32_t level)
 	return 0;
 }
 
-/* set level */
+int
+rte_log_set_level_match(const char *pattern, uint32_t level)
+{
+	size_t i;
+
+	if (level > RTE_LOG_DEBUG)
+		return -1;
+
+	for (i = 0; i < rte_logs.dynamic_types_len; i++) {
+		if (rte_logs.dynamic_types[i].name == NULL)
+			continue;
+
+		if (fnmatch(pattern, rte_logs.dynamic_types[i].name, 0))
+			rte_logs.dynamic_types[i].loglevel = level;
+	}
+
+	return 0;
+}
+
+/* set level by regular expression (using pattern match is preferred) */
 int
 rte_log_set_level_regexp(const char *pattern, uint32_t level)
 {
lib/librte_eal/common/eal_common_options.c +24 -12
index f2339c3907e4..e8d832cc694a 100644
@@ -955,31 +955,43 @@ eal_parse_log_priority(const char *level)
 static int
 eal_parse_log_level(const char *arg)
 {
-	char *str, *type, *level;
+	char *str, *level;
+	const char *regex = NULL;
+	const char *pattern = NULL;
 	int priority;
 
 	str = strdup(arg);
 	if (str == NULL)
 		return -1;
 
-	if (strchr(str, ',') == NULL) {
-		type = NULL;
-		level = str;
+	if ((level = strchr(str, ','))) {
+		regex = str;
+		*level++ = '\0';
+	} else if ((level = strchr(str, ':'))) {
+		pattern = str;
+		*level++ = '\0';
 	} else {
-		type = strsep(&str, ",");
-		level = strsep(&str, ",");
+		level = str;
 	}
 
 	priority = eal_parse_log_priority(level);
 	if (priority < 0)
 		goto fail;
 	
-	if (type == NULL) {
+	if (regex) {
+		if (rte_log_set_level_regexp(regex, priority) < 0) {
+			fprintf(stderr, "cannot set log level %s,%d\n",
+				pattern, priority);
+			goto fail;
+		}
+	} else if (pattern) {
+		if (rte_log_set_level_match(pattern, priority) < 0) {
+			fprintf(stderr, "cannot set log level %s:%d\n",
+				pattern, priority);
+			goto fail;
+		}
+	} else {
 		rte_log_set_global_level(priority);
-	} else if (rte_log_set_level_regexp(type, priority) < 0) {
-		fprintf(stderr, "cannot set log level %s,%d\n",
-			type, priority);
-		goto fail;
 	}
 
 	free(str);

@@ -1336,7 +1348,7 @@ eal_common_usage(void)
 	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"=<int>   Set global log level\n"
-	       "  --"OPT_LOG_LEVEL"=<type-regexp>,<int>\n"
+	       "  --"OPT_LOG_LEVEL"=<type-match>:<int>\n"
 	       "                      Set specific log level\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
lib/librte_eal/common/include/rte_log.h +14 -2
index 9029c7856d31..6d0ff9fe4623 100644
@@ -132,13 +132,25 @@ int rte_log_get_level(uint32_t logtype);
  * Set the log level for a given type.
  *
  * @param pattern
- *   The regexp identifying the log type.
+ *   The match pattern identifying the log type.
  * @param level
  *   The level to be set.
  * @return
  *   0 on success, a negative value if level is invalid.
  */
-int rte_log_set_level_regexp(const char *pattern, uint32_t level);
+int rte_log_set_level_match(const char *pattern, uint32_t level);
+
+/**
+ * Set the log level for a given type.
+ *
+ * @param regex
+ *   The regular expression identifying the log type.
+ * @param level
+ *   The level to be set.
+ * @return
+ *   0 on success, a negative value if level is invalid.
+ */
+int rte_log_set_level_regexp(const char *regex, uint32_t level);
 
 /**
  * Set the log level for a given type.
Signature · 1 line
2.16.1
author
#3 lore raw

PATCH 3/3 doc: update log level matching in documentation

In reply to Stephen Hemminger

The use of dynamic log in documentation should use matching not regex notation.

Signed-off-by Stephen Hemminger <[email protected]>
---
 doc/guides/contributing/coding_style.rst | 2 +-
 doc/guides/nics/qede.rst                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
doc/guides/contributing/coding_style.rst +1 -1
index b0f0adb887b4..365a4b17a983 100644
@@ -615,7 +615,7 @@ In the DPDK environment, use the logging interface provided:
  rte_log_set_level(my_logtype2, RTE_LOG_NOTICE);
 
  /* enable all PMD logs (whose identifier string starts with "pmd") */
- rte_log_set_level_regexp("pmd.*", RTE_LOG_DEBUG);
+ rte_log_set_level_match("pmd.*", RTE_LOG_DEBUG);
 
  /* log in debug level */
  rte_log_set_global_level(RTE_LOG_DEBUG);
doc/guides/nics/qede.rst +1 -1
index 63ce9b4c60c6..42dd70db39df 100644
@@ -193,7 +193,7 @@ This section provides instructions to configure SR-IOV with Linux OS.
 
 
 #. Running testpmd
-   (Supply ``--log-level="pmd.net.qede.driver",7`` to view informational messages):
+   (Supply ``--log-level="pmd.net.qede.driver:7`` to view informational messages):
 
    Refer to the document
    :ref:`compiling and testing a PMD for a NIC <pmd_build_and_test>` to run
Signature · 1 line
2.16.1
author
#4 lore raw

v2 cover logging enhancements

In reply to Stephen Hemminger

The current dynamic logging has some awkward user interface choices. It uses integers for log levels which requires user to know the mapping between numeric and symbolic values.

A bigger problem was the choice of regular expressions and option format for dynamic logging. Dynamic log names are seperated with a period and the wildcard character for regular expressions is a period. It is just a happy accident the expressions like:

        "pmd.net.virtio.*"

work as expected. This patch set adds a more usable solution with filename style matching.

Also, the choice of comma as seperator for log-level option was not consistent with other options. For other options, comma is used to seperate list of equal values as in:

        -l 1,2,3

Since new match required a backwards compatiable option the colon is now used to seperate name and value.

So:

        --log-level='pmd.net.virtio.*,7'
still works as expected. But the prefered syntax is:
        --log-level='pmd.net.virtio.*:info'

If this is accepted, I think we should mark the old regex style matching as deprecated and remove it in 18.11??

Also, the dynamic log level pattern stuff is not adaquately documented right now. There are only a couple of vague references in the current documentation (which this updates).

v2
- whitespace checkpatch revisions

Stephen Hemminger (3):
eal: allow symbolic log levels
log: add ability to match dynamic log based on shell pattern
doc: update log level matching in documentation

 doc/guides/contributing/coding_style.rst   |  2 +-
 doc/guides/nics/qede.rst                   |  2 +-
 lib/librte_eal/common/eal_common_log.c     | 22 +++++++-
 lib/librte_eal/common/eal_common_options.c | 86 +++++++++++++++++++++++-------
 lib/librte_eal/common/include/rte_log.h    | 16 +++++-
 5 files changed, 103 insertions(+), 25 deletions(-)
Signature · 1 line
2.16.1
author
#5 lore raw

PATCH v2 1/3 eal: allow symbolic log levels

Much easeier to remember names than numbers. Allows

	--log-level=pmd.net.ixgbe.*,debug
Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/librte_eal/common/eal_common_options.c | 66 ++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 16 deletions(-)
lib/librte_eal/common/eal_common_options.c +50 -16
index 9f2f8d25afd9..19069638ea05 100644
@@ -911,10 +911,52 @@ eal_parse_syslog(const char *facility, struct internal_config *conf)
 }
 
 static int
-eal_parse_log_level(const char *arg)
+eal_parse_log_priority(const char *level)
 {
-	char *end, *str, *type, *level;
+	static const char *levels[] = {
+		[RTE_LOG_EMERG]   = "emergency",
+		[RTE_LOG_ALERT]   = "alert",
+		[RTE_LOG_CRIT]    = "critical",
+		[RTE_LOG_ERR]     = "error",
+		[RTE_LOG_WARNING] = "warning",
+		[RTE_LOG_NOTICE]  = "notice",
+		[RTE_LOG_INFO]    = "info",
+		[RTE_LOG_DEBUG]   = "debug",
+	};
+	size_t len = strlen(level);
 	unsigned long tmp;
+	char *end;
+	unsigned int i;
+
+	if (len == 0)
+		return -1;
+
+	/* look for named values, skip 0 which is not a valid level */
+	for (i = 1; i < sizeof(levels) / sizeof(levels[0]); i++) {
+		if (strncmp(levels[i], level, len) == 0)
+			return i;
+	}
+
+	/* not a string, maybe it is numeric */
+	errno = 0;
+	tmp = strtoul(level, &end, 0);
+
+	/* check for errors */
+	if (errno != 0 || end == NULL || *end != '\0')
+		return -1;
+
+	/* log_level is a uint32_t */
+	if (tmp >= UINT32_MAX)
+		return -1;
+
+	return tmp;
+}
+
+static int
+eal_parse_log_level(const char *arg)
+{
+	char *str, *type, *level;
+	int priority;
 
 	str = strdup(arg);
 	if (str == NULL)

@@ -928,23 +970,15 @@ eal_parse_log_level(const char *arg)
 		level = strsep(&str, ",");
 	}
 
-	errno = 0;
-	tmp = strtoul(level, &end, 0);
-
-	/* check for errors */
-	if ((errno != 0) || (level[0] == '\0') ||
-		    end == NULL || (*end != '\0'))
-		goto fail;
-
-	/* log_level is a uint32_t */
-	if (tmp >= UINT32_MAX)
+	priority = eal_parse_log_priority(level);
+	if (priority < 0)
 		goto fail;
 
 	if (type == NULL) {
-		rte_log_set_global_level(tmp);
-	} else if (rte_log_set_level_regexp(type, tmp) < 0) {
-		printf("cannot set log level %s,%lu\n",
-			type, tmp);
+		rte_log_set_global_level(priority);
+	} else if (rte_log_set_level_regexp(type, priority) < 0) {
+		fprintf(stderr, "cannot set log level %s,%d\n",
+			type, priority);
 		goto fail;
 	}
Signature · 1 line
2.16.1
author
#6 lore raw

PATCH v2 2/3 log: add ability to match dynamic log based on shell pattern

In reply to Stephen Hemminger

Regular expressions are not the best way to match a hierarchical pattern like dynamic log levels. And the separator for dynamic log levels is period which is the regex wildcard character.

A better solution is to use filename matching 'globbing' so that log levels match like file paths. For compatibility, use colon to separate pattern match style arguments. For example:

	--log-level 'pmd.net.virtio.*:debug'
Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/librte_eal/common/eal_common_log.c     | 22 +++++++++++++++++-
 lib/librte_eal/common/eal_common_options.c | 36 ++++++++++++++++++++----------
 lib/librte_eal/common/include/rte_log.h    | 16 +++++++++++--
 3 files changed, 59 insertions(+), 15 deletions(-)
lib/librte_eal/common/eal_common_log.c +21 -1
index 37b2e20e539b..2601c7cd4e4f 100644
@@ -9,6 +9,7 @@
 #include <string.h>
 #include <errno.h>
 #include <regex.h>
+#include <fnmatch.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>

@@ -89,7 +90,26 @@ rte_log_set_level(uint32_t type, uint32_t level)
 	return 0;
 }
 
-/* set level */
+int
+rte_log_set_level_match(const char *pattern, uint32_t level)
+{
+	size_t i;
+
+	if (level > RTE_LOG_DEBUG)
+		return -1;
+
+	for (i = 0; i < rte_logs.dynamic_types_len; i++) {
+		if (rte_logs.dynamic_types[i].name == NULL)
+			continue;
+
+		if (fnmatch(pattern, rte_logs.dynamic_types[i].name, 0))
+			rte_logs.dynamic_types[i].loglevel = level;
+	}
+
+	return 0;
+}
+
+/* set level by regular expression (using pattern match is preferred) */
 int
 rte_log_set_level_regexp(const char *pattern, uint32_t level)
 {
lib/librte_eal/common/eal_common_options.c +24 -12
index 19069638ea05..45a75f75ae94 100644
@@ -955,31 +955,43 @@ eal_parse_log_priority(const char *level)
 static int
 eal_parse_log_level(const char *arg)
 {
-	char *str, *type, *level;
+	char *str, *level;
+	const char *regex = NULL;
+	const char *pattern = NULL;
 	int priority;
 
 	str = strdup(arg);
 	if (str == NULL)
 		return -1;
 
-	if (strchr(str, ',') == NULL) {
-		type = NULL;
-		level = str;
+	if ((level = strchr(str, ','))) {
+		regex = str;
+		*level++ = '\0';
+	} else if ((level = strchr(str, ':'))) {
+		pattern = str;
+		*level++ = '\0';
 	} else {
-		type = strsep(&str, ",");
-		level = strsep(&str, ",");
+		level = str;
 	}
 
 	priority = eal_parse_log_priority(level);
 	if (priority < 0)
 		goto fail;
 
-	if (type == NULL) {
+	if (regex) {
+		if (rte_log_set_level_regexp(regex, priority) < 0) {
+			fprintf(stderr, "cannot set log level %s,%d\n",
+				pattern, priority);
+			goto fail;
+		}
+	} else if (pattern) {
+		if (rte_log_set_level_match(pattern, priority) < 0) {
+			fprintf(stderr, "cannot set log level %s:%d\n",
+				pattern, priority);
+			goto fail;
+		}
+	} else {
 		rte_log_set_global_level(priority);
-	} else if (rte_log_set_level_regexp(type, priority) < 0) {
-		fprintf(stderr, "cannot set log level %s,%d\n",
-			type, priority);
-		goto fail;
 	}
 
 	free(str);

@@ -1336,7 +1348,7 @@ eal_common_usage(void)
 	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"=<int>   Set global log level\n"
-	       "  --"OPT_LOG_LEVEL"=<type-regexp>,<int>\n"
+	       "  --"OPT_LOG_LEVEL"=<type-match>:<int>\n"
 	       "                      Set specific log level\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
lib/librte_eal/common/include/rte_log.h +14 -2
index 9029c7856d31..6d0ff9fe4623 100644
@@ -132,13 +132,25 @@ int rte_log_get_level(uint32_t logtype);
  * Set the log level for a given type.
  *
  * @param pattern
- *   The regexp identifying the log type.
+ *   The match pattern identifying the log type.
  * @param level
  *   The level to be set.
  * @return
  *   0 on success, a negative value if level is invalid.
  */
-int rte_log_set_level_regexp(const char *pattern, uint32_t level);
+int rte_log_set_level_match(const char *pattern, uint32_t level);
+
+/**
+ * Set the log level for a given type.
+ *
+ * @param regex
+ *   The regular expression identifying the log type.
+ * @param level
+ *   The level to be set.
+ * @return
+ *   0 on success, a negative value if level is invalid.
+ */
+int rte_log_set_level_regexp(const char *regex, uint32_t level);
 
 /**
  * Set the log level for a given type.
Signature · 1 line
2.16.1
#7 lore raw

Re: [PATCH v2 2/3] log: add ability to match dynamic log based on shell pattern

23/02/2018 22:17, Stephen Hemminger:

Quoted 14 lines

Regular expressions are not the best way to match a hierarchical pattern like dynamic log levels. And the separator for dynamic log levels is period which is the regex wildcard character.

A better solution is to use filename matching 'globbing' so that log levels match like file paths. For compatibility, use colon to separate pattern match style arguments. For example:

	--log-level 'pmd.net.virtio.*:debug'
Signed-off-by Stephen Hemminger <[email protected]>

---

lib/librte_eal/common/eal_common_log.c +2 -0
+int
+rte_log_set_level_match(const char *pattern, uint32_t level)

[...]

Quoted 3 lines

+/* set level by regular expression (using pattern match is preferred) */
int
rte_log_set_level_regexp(const char *pattern, uint32_t level)

I think "pattern" is more appropriate than "match" to differentiate from "regexp". So I suggest this function name:
rte_log_set_level_pattern

#8 lore raw

Re: [PATCH v2 2/3] log: add ability to match dynamic log based on shell pattern

Hi Stephen,

04/04/2018 13:34, Thomas Monjalon:

Quoted 16 lines

23/02/2018 22:17, Stephen Hemminger:

Quoted 14 lines

Regular expressions are not the best way to match a hierarchical pattern like dynamic log levels. And the separator for dynamic log levels is period which is the regex wildcard character.

A better solution is to use filename matching 'globbing' so that log levels match like file paths. For compatibility, use colon to separate pattern match style arguments. For example:

	--log-level 'pmd.net.virtio.*:debug'
Signed-off-by Stephen Hemminger <[email protected]>

---

lib/librte_eal/common/eal_common_log.c +2 -0
+int
+rte_log_set_level_match(const char *pattern, uint32_t level)

[...]

Quoted 7 lines
Quoted 3 lines

+/* set level by regular expression (using pattern match is preferred) */
int
rte_log_set_level_regexp(const char *pattern, uint32_t level)

I think "pattern" is more appropriate than "match" to differentiate from "regexp". So I suggest this function name:
rte_log_set_level_pattern

Are you OK to do a v3 with this change?

author
#9 lore raw

Re: [PATCH v2 2/3] log: add ability to match dynamic log based on shell pattern

Quoted 23 lines · On Mon, 23 Apr 2018 23:08:01 +0200 Thomas Monjalon <[email protected]> wrote:

Hi Stephen,

04/04/2018 13:34, Thomas Monjalon:

Quoted 20 lines

23/02/2018 22:17, Stephen Hemminger:

Quoted 14 lines

Regular expressions are not the best way to match a hierarchical pattern like dynamic log levels. And the separator for dynamic log levels is period which is the regex wildcard character.

A better solution is to use filename matching 'globbing' so that log levels match like file paths. For compatibility, use colon to separate pattern match style arguments. For example:

	--log-level 'pmd.net.virtio.*:debug'
Signed-off-by Stephen Hemminger <[email protected]>

---

lib/librte_eal/common/eal_common_log.c +2 -0
+int
+rte_log_set_level_match(const char *pattern, uint32_t level)

[...]

Quoted 3 lines

+/* set level by regular expression (using pattern match is preferred) */
int
rte_log_set_level_regexp(const char *pattern, uint32_t level)

Quoted 7 lines
Quoted 3 lines

I think "pattern" is more appropriate than "match" to differentiate from "regexp". So I suggest this function name:
rte_log_set_level_pattern

Are you OK to do a v3 with this change?

Sure, I expected some feedback since it it was a semantic change in the API. Let me also recheck the documentation.

author
#10 lore raw

PATCH v2 3/3 doc: update log level matching in documentation

In reply to Stephen Hemminger

The use of dynamic log in documentation should use matching not regex notation.

Signed-off-by Stephen Hemminger <[email protected]>
---
 doc/guides/contributing/coding_style.rst | 2 +-
 doc/guides/nics/qede.rst                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
doc/guides/contributing/coding_style.rst +1 -1
index b0f0adb887b4..365a4b17a983 100644
@@ -615,7 +615,7 @@ In the DPDK environment, use the logging interface provided:
  rte_log_set_level(my_logtype2, RTE_LOG_NOTICE);
 
  /* enable all PMD logs (whose identifier string starts with "pmd") */
- rte_log_set_level_regexp("pmd.*", RTE_LOG_DEBUG);
+ rte_log_set_level_match("pmd.*", RTE_LOG_DEBUG);
 
  /* log in debug level */
  rte_log_set_global_level(RTE_LOG_DEBUG);
doc/guides/nics/qede.rst +1 -1
index 63ce9b4c60c6..42dd70db39df 100644
@@ -193,7 +193,7 @@ This section provides instructions to configure SR-IOV with Linux OS.
 
 
 #. Running testpmd
-   (Supply ``--log-level="pmd.net.qede.driver",7`` to view informational messages):
+   (Supply ``--log-level="pmd.net.qede.driver:7`` to view informational messages):
 
    Refer to the document
    :ref:`compiling and testing a PMD for a NIC <pmd_build_and_test>` to run
Signature · 1 line
2.16.1
#11 lore raw

Re: [PATCH v2 3/3] doc: update log level matching in documentation

23/02/2018 22:17, Stephen Hemminger:

Quoted 8 lines

The use of dynamic log in documentation should use matching not regex notation.

Signed-off-by Stephen Hemminger <[email protected]>
---
 doc/guides/contributing/coding_style.rst | 2 +-
 doc/guides/nics/qede.rst                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

This patch can be merged with previous one (related code change).

#12 lore raw

Re: [PATCH v2 0/3] logging enhancements

In reply to Stephen Hemminger

23/02/2018 22:17, Stephen Hemminger:

Quoted 15 lines

The current dynamic logging has some awkward user interface choices. It uses integers for log levels which requires user to know the mapping between numeric and symbolic values.

A bigger problem was the choice of regular expressions and option format for dynamic logging. Dynamic log names are seperated with a period and the wildcard character for regular expressions is a period. It is just a happy accident the expressions like:

        "pmd.net.virtio.*"

work as expected. This patch set adds a more usable solution with filename style matching.

Also, the choice of comma as seperator for log-level option was not consistent with other options. For other options, comma is used to seperate list of equal values as in:

Quoted 8 lines
        -l 1,2,3

Since new match required a backwards compatiable option the colon is now used to seperate name and value.

So:

        --log-level='pmd.net.virtio.*,7'
still works as expected. But the prefered syntax is:
        --log-level='pmd.net.virtio.*:info'

+1
This syntax looks better.

Quoted 2 lines

If this is accepted, I think we should mark the old regex style matching as deprecated and remove it in 18.11??

Good question. Do we want to deprecate the regex style?

Quoted 3 lines

Also, the dynamic log level pattern stuff is not adaquately documented right now. There are only a couple of vague references in the current documentation (which this updates).

I think you should document the symbolic log levels in the guide and in the --help.

#13 lore raw

Re: [PATCH v2 0/3] logging enhancements

Quoted 20 lines · Thomas Monjalon

23/02/2018 22:17, Stephen Hemminger:

Quoted 19 lines

The current dynamic logging has some awkward user interface choices. It uses integers for log levels which requires user to know the mapping between numeric and symbolic values.

A bigger problem was the choice of regular expressions and option format for dynamic logging. Dynamic log names are seperated with a period and the wildcard character for regular expressions is a period. It is just a happy accident the expressions like:

        "pmd.net.virtio.*"

work as expected. This patch set adds a more usable solution with filename style matching.

Also, the choice of comma as seperator for log-level option was not consistent with other options. For other options, comma is used to seperate list of equal values as in:

        -l 1,2,3

Since new match required a backwards compatiable option the colon is now used to seperate name and value.

Quoted 7 lines
Quoted 4 lines

So:

        --log-level='pmd.net.virtio.*,7'
still works as expected. But the prefered syntax is:
        --log-level='pmd.net.virtio.*:info'

+1
This syntax looks better.

Agree, this is easier to use and understand.