[PATCH 0/3] logging enhancments

[PATCH 0/3] logging enhancments

[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).

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

-- 
2.16.1

[PATCH 1/3] eal: allow symbolic log levels

[Stephen Hemminger]

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 <stephen@networkplumber.org>
---
 lib/librte_eal/common/eal_common_options.c | 68 ++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 9f2f8d25afd9..f2339c3907e4 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -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;
 	}
 
-- 
2.16.1

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

[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 <stephen@networkplumber.org>
---
 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(-)

diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 37b2e20e539b..2601c7cd4e4f 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -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)
 {
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index f2339c3907e4..e8d832cc694a 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -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"
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 9029c7856d31..6d0ff9fe4623 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -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.
-- 
2.16.1

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

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 doc/guides/contributing/coding_style.rst | 2 +-
 doc/guides/nics/qede.rst                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index b0f0adb887b4..365a4b17a983 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -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);
diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
index 63ce9b4c60c6..42dd70db39df 100644
--- a/doc/guides/nics/qede.rst
+++ b/doc/guides/nics/qede.rst
@@ -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
-- 
2.16.1

[PATCH v2 0/3] logging enhancements

[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(-)

-- 
2.16.1

[PATCH v2 1/3] eal: allow symbolic log levels

[Stephen Hemminger]

Much easeier to remember names than numbers. Allows
	--log-level=pmd.net.ixgbe.*,debug

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/common/eal_common_options.c | 66 ++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 16 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 9f2f8d25afd9..19069638ea05 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -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;
 	}
 
-- 
2.16.1

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

[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 <stephen@networkplumber.org>
---
 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(-)

diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 37b2e20e539b..2601c7cd4e4f 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -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)
 {
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 19069638ea05..45a75f75ae94 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -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"
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 9029c7856d31..6d0ff9fe4623 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -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.
-- 
2.16.1

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

[Thomas Monjalon]

23/02/2018 22:17, 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 <stephen@networkplumber.org>
> ---
> +int
> +rte_log_set_level_match(const char *pattern, uint32_t level)
[...]
> +/* 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

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

[Thomas Monjalon]

Hi Stephen,

04/04/2018 13:34, Thomas Monjalon:
> 23/02/2018 22:17, 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 <stephen@networkplumber.org>
> > ---
> > +int
> > +rte_log_set_level_match(const char *pattern, uint32_t level)
> [...]
> > +/* 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?

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

[Stephen Hemminger]

On Mon, 23 Apr 2018 23:08:01 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> Hi Stephen,
> 
> 04/04/2018 13:34, Thomas Monjalon:
> > 23/02/2018 22:17, 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 <stephen@networkplumber.org>
> > > ---
> > > +int
> > > +rte_log_set_level_match(const char *pattern, uint32_t level)  
> > [...]  
> > > +/* 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?
> 
> 

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

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

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 doc/guides/contributing/coding_style.rst | 2 +-
 doc/guides/nics/qede.rst                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index b0f0adb887b4..365a4b17a983 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -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);
diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
index 63ce9b4c60c6..42dd70db39df 100644
--- a/doc/guides/nics/qede.rst
+++ b/doc/guides/nics/qede.rst
@@ -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
-- 
2.16.1

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

[Thomas Monjalon]

23/02/2018 22:17, Stephen Hemminger:
> The use of dynamic log in documentation should use matching not
> regex notation.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  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).

Re: [PATCH v2 0/3] logging enhancements

[Thomas Monjalon]

23/02/2018 22:17, 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'

+1
This syntax looks better.

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

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

Re: [PATCH v2 0/3] logging enhancements

[Olivier Matz]

On Wed, Apr 04, 2018 at 01:37:37PM +0200, Thomas Monjalon wrote:
> 23/02/2018 22:17, 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'
> 
> +1
> This syntax looks better.

Agree, this is easier to use and understand.