Change in libosmocore[master]: logging vty: rewrite 'logging level' vty cmd generation

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Wed Sep 12 01:50:15 UTC 2018


Neels Hofmeyr has uploaded this change for review. ( https://gerrit.osmocom.org/10884


Change subject: logging vty: rewrite 'logging level' vty cmd generation
......................................................................

logging vty: rewrite 'logging level' vty cmd generation

Completely drop the implementations of log_vty_command_{str,description}().
These functions have been public API once, marked as deprecated since
c65c5b4ea075ef6cef11fff9442ae0b15c1d6af7 (March 2017). I considered to keep
them, or reduce them to useless stubs, but it is quite silly, really. These
functions are completely and utterly useless outside of libosmocore. Any
program linking these deserves to fail.

Re-implement vty logging level command gen, in logging_vty.c. logging.c is
simply the wrong place for that.

Introduce logging_internal.h to share logging definitions to logging_vty.c
without publishing as API.

Introduce static gen_logging_level_cmd_strs() to compose a list of category
arguments with their descriptions for VTY commands. Use osmo_talloc_asprintf()
instead of the previous error prone and chaotic strlen() counting method.

Do not dynamically generate log level arguments, just keep static strings. We
are super unlikely to ever change the log levels we have.

No changes in logging_vty_test.vty: proves that there is no functional change.

All of this, besides introducing basic sanity, is cosmetic preparation to be
able to re-use the generic command generation code for arbitrary commands with
category or level args (for deprecated and new keywords).

Rationale: I want to hide 'all' and 'everything' from the VTY command
documentation, by means of deprecating. I first tried to simply define a
deprecated 'logging level CAT everything' command:

  logging level (all|rsl|rr|...) (debug|info|notice|error|fatal)
  logging level CAT everything                   # <- deprecated and hidden

But unfortunately, command matching doesn't work as intended when the CAT
argument reflects a valid category; I want it to invoke the deprecated function
as soon as the 'everything' keyword follows, but it stays stuck to the "valid"
command when the category argument matches an explicit keyword in that list,
and will throw an error on the following 'everything' keyword. I.e.:

  logging level rsl everything
  % Unknown command  # <-- leads to config file parse error

  logging level unknown_string everything
  % Ignoring deprecated 'everything'  # <-- works only for invalid categories

So I need to define 'everything' separately, again with a list of each valid
category instead of a generic CAT arg.

Change-Id: I3b083f27e3d751ccec258880ae7676e9af959a63
---
M include/Makefile.am
M include/osmocom/core/logging.h
A include/osmocom/core/logging_internal.h
M src/logging.c
M src/vty/logging_vty.c
5 files changed, 82 insertions(+), 172 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/84/10884/1

diff --git a/include/Makefile.am b/include/Makefile.am
index ef8ec65..19695d1 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -152,7 +152,10 @@
 endif
 
 noinst_HEADERS = \
-	osmocom/gsm/kasumi.h osmocom/gsm/gea.h
+	osmocom/gsm/kasumi.h \
+	osmocom/gsm/gea.h \
+	osmocom/core/logging_internal.h \
+	$(NULL)
 
 osmocom/core/bit%gen.h: osmocom/core/bitXXgen.h.tpl
 	$(AM_V_GEN)$(MKDIR_P) $(dir $@)
diff --git a/include/osmocom/core/logging.h b/include/osmocom/core/logging.h
index c60143d..295e5a8 100644
--- a/include/osmocom/core/logging.h
+++ b/include/osmocom/core/logging.h
@@ -374,10 +374,6 @@
 void log_add_target(struct log_target *target);
 void log_del_target(struct log_target *target);
 
-/* Generate command string for VTY use */
-const char *log_vty_command_string() OSMO_DEPRECATED_OUTSIDE_LIBOSMOCORE;
-const char *log_vty_command_description() OSMO_DEPRECATED_OUTSIDE_LIBOSMOCORE;
-
 struct log_target *log_target_find(int type, const char *fname);
 extern struct llist_head osmo_log_target_list;
 
diff --git a/include/osmocom/core/logging_internal.h b/include/osmocom/core/logging_internal.h
new file mode 100644
index 0000000..55b1bbd
--- /dev/null
+++ b/include/osmocom/core/logging_internal.h
@@ -0,0 +1,14 @@
+#pragma once
+
+/*! \defgroup logging_internal Osmocom logging internals
+ *  @{
+ * \file logging_internal.h */
+
+#include <osmocom/core/utils.h>
+
+extern void *tall_log_ctx;
+extern const struct log_info *osmo_log_info;
+
+void assert_loginfo(const char *src);
+
+/*! @} */
diff --git a/src/logging.c b/src/logging.c
index de0f2b0..7c2d61f 100644
--- a/src/logging.c
+++ b/src/logging.c
@@ -35,7 +35,6 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
-#include <ctype.h>
 
 #ifdef HAVE_STRINGS_H
 #include <strings.h>
@@ -61,12 +60,10 @@
 struct log_info *osmo_log_info;
 
 static struct log_context log_context;
-static void *tall_log_ctx = NULL;
+void *tall_log_ctx = NULL;
 LLIST_HEAD(osmo_log_target_list);
 
-#define LOGLEVEL_DEFS	6	/* Number of loglevels.*/
-
-static const struct value_string loglevel_strs[LOGLEVEL_DEFS+1] = {
+static const struct value_string loglevel_strs[] = {
 	{ 0,		"EVERYTHING" },
 	{ LOGL_DEBUG,	"DEBUG" },
 	{ LOGL_INFO,	"INFO" },
@@ -175,19 +172,7 @@
 	},
 };
 
-/*! descriptive string for each log level */
-/* You have to keep this in sync with the structure loglevel_strs. */
-static const char *loglevel_descriptions[LOGLEVEL_DEFS+1] = {
-	"Don't use. It doesn't log anything",
-	"Log debug messages and higher levels",
-	"Log informational messages and higher levels",
-	"Log noticeable messages and higher levels",
-	"Log error messages and higher levels",
-	"Log only fatal messages",
-	NULL,
-};
-
-static void assert_loginfo(const char *src)
+void assert_loginfo(const char *src)
 {
 	if (!osmo_log_info) {
 		fprintf(stderr, "ERROR: osmo_log_info == NULL! "
@@ -963,149 +948,6 @@
 	return rc;
 }
 
-/*! Generates the logging command string for VTY
- *  \param[in] unused_info Deprecated parameter, no longer used!
- *  \returns vty command string for use by VTY command node
- */
-const char *log_vty_command_string()
-{
-	struct log_info *info = osmo_log_info;
-	int len = 0, offset = 0, ret, i, rem;
-	int size = strlen("logging level (all|) ()") + 1;
-	char *str;
-
-	assert_loginfo(__func__);
-
-	for (i = 0; i < info->num_cat; i++) {
-		if (info->cat[i].name == NULL)
-			continue;
-		size += strlen(info->cat[i].name) + 1;
-	}
-
-	for (i = 0; i < LOGLEVEL_DEFS; i++)
-		size += strlen(loglevel_strs[i].str) + 1;
-
-	rem = size;
-	str = talloc_zero_size(tall_log_ctx, size);
-	if (!str)
-		return NULL;
-
-	ret = snprintf(str + offset, rem, "logging level (all|");
-	if (ret < 0)
-		goto err;
-	OSMO_SNPRINTF_RET(ret, rem, offset, len);
-
-	for (i = 0; i < info->num_cat; i++) {
-		if (info->cat[i].name) {
-			int j, name_len = strlen(info->cat[i].name)+1;
-			char name[name_len];
-
-			for (j = 0; j < name_len; j++)
-				name[j] = tolower((unsigned char)info->cat[i].name[j]);
-
-			name[name_len-1] = '\0';
-			ret = snprintf(str + offset, rem, "%s|", name+1);
-			if (ret < 0)
-				goto err;
-			OSMO_SNPRINTF_RET(ret, rem, offset, len);
-		}
-	}
-	offset--;	/* to remove the trailing | */
-	rem++;
-
-	ret = snprintf(str + offset, rem, ") (");
-	if (ret < 0)
-		goto err;
-	OSMO_SNPRINTF_RET(ret, rem, offset, len);
-
-	for (i = 0; i < LOGLEVEL_DEFS; i++) {
-		int j, loglevel_str_len = strlen(loglevel_strs[i].str)+1;
-		char loglevel_str[loglevel_str_len];
-
-		for (j = 0; j < loglevel_str_len; j++)
-			loglevel_str[j] = tolower((unsigned char)loglevel_strs[i].str[j]);
-
-		loglevel_str[loglevel_str_len-1] = '\0';
-		ret = snprintf(str + offset, rem, "%s|", loglevel_str);
-		if (ret < 0)
-			goto err;
-		OSMO_SNPRINTF_RET(ret, rem, offset, len);
-	}
-	offset--;	/* to remove the trailing | */
-	rem++;
-
-	ret = snprintf(str + offset, rem, ")");
-	if (ret < 0)
-		goto err;
-	OSMO_SNPRINTF_RET(ret, rem, offset, len);
-err:
-	str[size-1] = '\0';
-	return str;
-}
-
-/*! Generates the logging command description for VTY
- *  \param[in] unused_info Deprecated parameter, no longer used!
- *  \returns logging command description for use by VTY command node
- */
-const char *log_vty_command_description()
-{
-	struct log_info *info = osmo_log_info;
-	char *str;
-	int i, ret, len = 0, offset = 0, rem;
-	unsigned int size =
-		strlen(LOGGING_STR
-		       "Set the log level for a specified category\n") + 1;
-
-	assert_loginfo(__func__);
-
-	for (i = 0; i < info->num_cat; i++) {
-		if (info->cat[i].name == NULL)
-			continue;
-		size += strlen(info->cat[i].description) + 1;
-	}
-
-	for (i = 0; i < LOGLEVEL_DEFS; i++)
-		size += strlen(loglevel_descriptions[i]) + 1;
-
-	size += strlen("Global setting for all subsystems") + 1;
-	rem = size;
-	str = talloc_zero_size(tall_log_ctx, size);
-	if (!str)
-		return NULL;
-
-	ret = snprintf(str + offset, rem, LOGGING_STR
-			"Set the log level for a specified category\n");
-	if (ret < 0)
-		goto err;
-	OSMO_SNPRINTF_RET(ret, rem, offset, len);
-
-	ret = snprintf(str + offset, rem,
-			"Global setting for all subsystems\n");
-	if (ret < 0)
-		goto err;
-	OSMO_SNPRINTF_RET(ret, rem, offset, len);
-
-	for (i = 0; i < info->num_cat; i++) {
-		if (info->cat[i].name == NULL)
-			continue;
-		ret = snprintf(str + offset, rem, "%s\n",
-				info->cat[i].description);
-		if (ret < 0)
-			goto err;
-		OSMO_SNPRINTF_RET(ret, rem, offset, len);
-	}
-	for (i = 0; i < LOGLEVEL_DEFS; i++) {
-		ret = snprintf(str + offset, rem, "%s\n",
-				loglevel_descriptions[i]);
-		if (ret < 0)
-			goto err;
-		OSMO_SNPRINTF_RET(ret, rem, offset, len);
-	}
-err:
-	str[size-1] = '\0';
-	return str;
-}
-
 /*! Initialize the Osmocom logging core
  *  \param[in] inf Information regarding logging categories
  *  \param[in] ctx \ref talloc context for logging allocations
diff --git a/src/vty/logging_vty.c b/src/vty/logging_vty.c
index 7d97bb1..2b001bc 100644
--- a/src/vty/logging_vty.c
+++ b/src/vty/logging_vty.c
@@ -28,6 +28,7 @@
 
 #include <osmocom/core/talloc.h>
 #include <osmocom/core/logging.h>
+#include <osmocom/core/logging_internal.h>
 #include <osmocom/core/utils.h>
 #include <osmocom/core/strrb.h>
 #include <osmocom/core/loggingrb.h>
@@ -40,6 +41,19 @@
 #include <osmocom/vty/logging.h>
 
 #define LOG_STR "Configure logging sub-system\n"
+#define LEVEL_STR "Set the log level for a specified category\n"
+
+#define CATEGORY_ALL_STR "Global setting for all subsystems\n"
+
+#define LOG_LEVEL_ARGS "debug|info|notice|error|fatal"
+#define LOG_LEVEL_STRS \
+	"Log debug messages and higher levels\n" \
+	"Log informational messages and higher levels\n" \
+	"Log noticeable messages and higher levels\n" \
+	"Log error messages and higher levels\n" \
+	"Log only fatal messages\n"
+
+#define EVERYTHING_STR "Don't use. It doesn't log anything\n"
 
 /*! \file logging_vty.c
  *  Configuration of logging from VTY
@@ -58,8 +72,6 @@
  *
  */
 
-extern const struct log_info *osmo_log_info;
-
 static void _vty_output(struct log_target *tgt,
 			unsigned int level, const char *line)
 {
@@ -268,6 +280,47 @@
 	return CMD_SUCCESS;
 }
 
+static void add_category_strings(char **cmd_str_p, char **doc_str_p,
+				 const struct log_info *categories)
+{
+	int i;
+	for (i = 0; i < categories->num_cat; i++) {
+		if (categories->cat[i].name == NULL)
+			continue;
+		/* skip the leading 'D' in each category name, hence '+ 1' */
+		osmo_talloc_asprintf(tall_log_ctx, *cmd_str_p, "%s%s",
+				     i ? "|" : "",
+				     osmo_str_tolower(categories->cat[i].name + 1));
+		osmo_talloc_asprintf(tall_log_ctx, *doc_str_p, "%s\n",
+				     categories->cat[i].description);
+	}
+}
+
+static void gen_logging_level_cmd_strs(struct cmd_element *cmd,
+				       const char *level_args, const char *level_strs)
+{
+	char *cmd_str = NULL;
+	char *doc_str = NULL;
+
+	assert_loginfo(__func__);
+
+	OSMO_ASSERT(cmd->string == NULL);
+	OSMO_ASSERT(cmd->doc == NULL);
+
+	osmo_talloc_asprintf(tall_log_ctx, cmd_str, "logging level (all|");
+	osmo_talloc_asprintf(tall_log_ctx, doc_str,
+			     LOGGING_STR
+			     LEVEL_STR
+			     CATEGORY_ALL_STR);
+	add_category_strings(&cmd_str, &doc_str, osmo_log_info);
+	osmo_talloc_asprintf(tall_log_ctx, cmd_str, ") %s", level_args);
+	osmo_talloc_asprintf(tall_log_ctx, doc_str, "%s", level_strs);
+
+	cmd->string = cmd_str;
+	cmd->doc = doc_str;
+}
+
+/* logging level (all|<categories>) (everything|debug|...|fatal) */
 DEFUN(logging_level,
       logging_level_cmd,
       NULL, /* cmdstr is dynamically set in logging_vty_add_cmds(). */
@@ -847,7 +900,7 @@
 				    name);
 	printf("%s\n", cmd->string);
 	cmd->func = log_deprecated_func;
-	cmd->doc = "Set the log level for a specified category\n"
+	cmd->doc = LEVEL_STR
 		   "Deprecated Category\n";
 	cmd->attr = CMD_ATTR_DEPRECATED;
 
@@ -871,9 +924,11 @@
 	install_element_ve(&logging_set_category_mask_cmd);
 	install_element_ve(&logging_set_category_mask_old_cmd);
 
-	/* Logging level strings are generated dynamically. */
-	logging_level_cmd.string = log_vty_command_string();
-	logging_level_cmd.doc = log_vty_command_description();
+	/* logging level (all|<categories>) (everything|debug|...|fatal) */
+	gen_logging_level_cmd_strs(&logging_level_cmd,
+				   "(everything|" LOG_LEVEL_ARGS ")",
+				   EVERYTHING_STR LOG_LEVEL_STRS);
+
 	install_element_ve(&logging_level_cmd);
 	install_element_ve(&show_logging_vty_cmd);
 	install_element_ve(&show_alarms_cmd);

-- 
To view, visit https://gerrit.osmocom.org/10884
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3b083f27e3d751ccec258880ae7676e9af959a63
Gerrit-Change-Number: 10884
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180912/1eb8de09/attachment.htm>


More information about the gerrit-log mailing list