[PATCH] libosmocore[master]: logging: centrally define ctx and filter indexes

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Fri Feb 17 16:30:08 UTC 2017


Review at  https://gerrit.osmocom.org/1844

logging: centrally define ctx and filter indexes

It is too easy for calling code to use the same filter and context indexes for
different filters and structs. For example, openbsc's IMSI filter and libgb's
GPRS_BVC filter both fall on index 1 even though there are plenty more indexes
to choose from. To alleviate this, have one central definition here, sort of
like ports.h does for VTY and CTRL port numbers.

Add static asserts to make sure the indexes fit in the available array and bit
mask space.

Calling code like openbsc.git and osmo-pcu need adjustments and/or should move
to using these enum values instead of their local definitions.

Include previous LOG_FILTER_ALL in the LOGGING_FILTER_* enum, and replace its
use by (1 << LOGGING_FILTER_ALL).

Change-Id: I5c343630020f4b108099696fd96c2111614c8067
---
M include/osmocom/core/logging.h
M include/osmocom/gprs/gprs_msgb.h
M src/gb/common_vty.c
M src/gb/common_vty.h
M src/gb/gprs_bssgp.c
M src/gb/gprs_bssgp_vty.c
M src/gb/gprs_ns.c
M src/gb/gprs_ns_vty.c
M src/logging.c
M src/vty/logging_vty.c
10 files changed, 64 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/44/1844/1

diff --git a/include/osmocom/core/logging.h b/include/osmocom/core/logging.h
index fcf77f0..509b570 100644
--- a/include/osmocom/core/logging.h
+++ b/include/osmocom/core/logging.h
@@ -89,8 +89,6 @@
 #define LOGL_ERROR	7	/*!< \brief error condition, requires user action */
 #define LOGL_FATAL	8	/*!< \brief fatal, program aborted */
 
-#define LOG_FILTER_ALL	0x0001
-
 /* logging levels defined by the library itself */
 #define DLGLOBAL	-1	/*!< global logging */
 #define DLLAPD		-2	/*!< LAPD implementation */
@@ -126,6 +124,21 @@
 	void *ctx[LOG_MAX_CTX+1];
 };
 
+enum logging_ctx_items {
+	LOGGING_CTX_GB_NSVC,
+	LOGGING_CTX_GB_BVC,
+	LOGGING_CTX_SUBSCR,
+	_LOGGING_CTX_COUNT
+};
+
+enum logging_filters {
+	LOGGING_FILTER_ALL,
+	LOGGING_FILTER_GB_NSVC,
+	LOGGING_FILTER_GB_BVC,
+	LOGGING_FILTER_SUBSCR,
+	_LOGGING_FILTER_COUNT
+};
+
 struct log_target;
 
 /*! \brief Log filter function */
diff --git a/include/osmocom/gprs/gprs_msgb.h b/include/osmocom/gprs/gprs_msgb.h
index 06f5cca..9ccc9a5 100644
--- a/include/osmocom/gprs/gprs_msgb.h
+++ b/include/osmocom/gprs/gprs_msgb.h
@@ -26,10 +26,6 @@
 #define msgb_bcid(__x)		LIBGB_MSGB_CB(__x)->bssgp_cell_id
 #define msgb_llch(__x)		LIBGB_MSGB_CB(__x)->llch
 
-/* logging contexts */
-#define GPRS_CTX_NSVC	0
-#define GPRS_CTX_BVC	1
-
 #include <osmocom/core/logging.h>
 int gprs_log_filter_fn(const struct log_context *ctx,
 			struct log_target *tar);
diff --git a/src/gb/common_vty.c b/src/gb/common_vty.c
index 5b20fcf..609ab3d 100644
--- a/src/gb/common_vty.c
+++ b/src/gb/common_vty.c
@@ -70,17 +70,17 @@
 int gprs_log_filter_fn(const struct log_context *ctx,
 			struct log_target *tar)
 {
-	const struct gprs_nsvc *nsvc = ctx->ctx[GPRS_CTX_NSVC];
-	const struct gprs_bvc *bvc = ctx->ctx[GPRS_CTX_BVC];
+	const struct gprs_nsvc *nsvc = ctx->ctx[LOGGING_CTX_GB_NSVC];
+	const struct gprs_bvc *bvc = ctx->ctx[LOGGING_CTX_GB_BVC];
 
 	/* Filter on the NS Virtual Connection */
-	if ((tar->filter_map & (1 << FLT_NSVC)) != 0
-	    && nsvc && (nsvc == tar->filter_data[FLT_NSVC]))
+	if ((tar->filter_map & (1 << LOGGING_FILTER_GB_NSVC)) != 0
+	    && nsvc && (nsvc == tar->filter_data[LOGGING_FILTER_GB_NSVC]))
 		return 1;
 
 	/* Filter on the NS Virtual Connection */
-	if ((tar->filter_map & (1 << FLT_BVC)) != 0
-	    && bvc && (bvc == tar->filter_data[FLT_BVC]))
+	if ((tar->filter_map & (1 << LOGGING_FILTER_GB_BVC)) != 0
+	    && bvc && (bvc == tar->filter_data[LOGGING_FILTER_GB_BVC]))
 		return 1;
 
 	return 0;
diff --git a/src/gb/common_vty.h b/src/gb/common_vty.h
index d8d0040..8c6b9ab 100644
--- a/src/gb/common_vty.h
+++ b/src/gb/common_vty.h
@@ -3,12 +3,6 @@
 
 extern int DNS, DBSSGP;
 
-enum log_filter {
-	_FLT_ALL = LOG_FILTER_ALL,	/* libosmocore */
-	FLT_NSVC = 1,
-	FLT_BVC  = 2,
-};
-
 extern struct cmd_element libgb_exit_cmd;
 extern struct cmd_element libgb_end_cmd;
 
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c
index 1ee942f..a402378 100644
--- a/src/gb/gprs_bssgp.c
+++ b/src/gb/gprs_bssgp.c
@@ -1049,7 +1049,7 @@
 	bctx = btsctx_by_bvci_nsei(bvci, msgb_nsei(msg));
 
 	if (bctx) {
-		log_set_context(GPRS_CTX_BVC, bctx);
+		log_set_context(LOGGING_CTX_GB_BVC, bctx);
 		rate_ctr_inc(&bctx->ctrg->ctr[BSSGP_CTR_PKTS_IN]);
 		rate_ctr_add(&bctx->ctrg->ctr[BSSGP_CTR_BYTES_IN],
 			     msgb_bssgp_len(msg));
diff --git a/src/gb/gprs_bssgp_vty.c b/src/gb/gprs_bssgp_vty.c
index 1f382a2..d8fc840 100644
--- a/src/gb/gprs_bssgp_vty.c
+++ b/src/gb/gprs_bssgp_vty.c
@@ -47,11 +47,11 @@
 				struct bssgp_bvc_ctx *bctx)
 {
 	if (bctx) {
-		target->filter_map |= (1 << FLT_BVC);
-		target->filter_data[FLT_BVC] = bctx;
-	} else if (target->filter_data[FLT_BVC]) {
-		target->filter_map = ~(1 << FLT_BVC);
-		target->filter_data[FLT_BVC] = NULL;
+		target->filter_map |= (1 << LOGGING_FILTER_GB_BVC);
+		target->filter_data[LOGGING_FILTER_GB_BVC] = bctx;
+	} else if (target->filter_data[LOGGING_FILTER_GB_BVC]) {
+		target->filter_map = ~(1 << LOGGING_FILTER_GB_BVC);
+		target->filter_data[LOGGING_FILTER_GB_BVC] = NULL;
 	}
 }
 
diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c
index 18845d4..28f5ff3 100644
--- a/src/gb/gprs_ns.c
+++ b/src/gb/gprs_ns.c
@@ -325,7 +325,7 @@
 {
 	int ret;
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 
 	/* Increment number of Uplink bytes */
 	rate_ctr_inc(&nsvc->ctrg->ctr[NS_CTR_PKTS_OUT]);
@@ -360,7 +360,7 @@
 	struct msgb *msg = gprs_ns_msgb_alloc();
 	struct gprs_ns_hdr *nsh;
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 
 	if (!msg)
 		return -ENOMEM;
@@ -384,7 +384,7 @@
 	uint16_t nsvci = htons(nsvc->nsvci);
 	uint16_t nsei = htons(nsvc->nsei);
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 
 	if (!msg)
 		return -ENOMEM;
@@ -416,7 +416,7 @@
 	struct gprs_ns_hdr *nsh;
 	uint16_t nsvci = htons(nsvc->nsvci);
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 
 	bvci = htons(bvci);
 
@@ -469,7 +469,7 @@
 	struct gprs_ns_hdr *nsh;
 	uint16_t nsvci = htons(nsvc->nsvci);
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 
 	if (!msg)
 		return -ENOMEM;
@@ -497,7 +497,7 @@
  */
 int gprs_ns_tx_unblock(struct gprs_nsvc *nsvc)
 {
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 	LOGP(DNS, LOGL_INFO, "NSEI=%u Tx NS UNBLOCK (NSVCI=%u)\n",
 		nsvc->nsei, nsvc->nsvci);
 
@@ -510,7 +510,7 @@
  */
 int gprs_ns_tx_alive(struct gprs_nsvc *nsvc)
 {
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 	LOGP(DNS, LOGL_DEBUG, "NSEI=%u Tx NS ALIVE (NSVCI=%u)\n",
 		nsvc->nsei, nsvc->nsvci);
 
@@ -523,7 +523,7 @@
  */
 int gprs_ns_tx_alive_ack(struct gprs_nsvc *nsvc)
 {
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 	LOGP(DNS, LOGL_DEBUG, "NSEI=%u Tx NS ALIVE_ACK (NSVCI=%u)\n",
 		nsvc->nsei, nsvc->nsvci);
 
@@ -548,7 +548,7 @@
 	enum ns_timeout tout = timer_mode_tout[mode];
 	unsigned int seconds = nsvc->nsi->timeout[tout];
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 	DEBUGP(DNS, "NSEI=%u Starting timer in mode %s (%u seconds)\n",
 		nsvc->nsei, get_value_string(timer_mode_strs, mode),
 		seconds);
@@ -576,7 +576,7 @@
 	enum ns_timeout tout = timer_mode_tout[nsvc->timer_mode];
 	unsigned int seconds = nsvc->nsi->timeout[tout];
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 	DEBUGP(DNS, "NSEI=%u Timer expired in mode %s (%u seconds)\n",
 		nsvc->nsei, get_value_string(timer_mode_strs, nsvc->timer_mode),
 		seconds);
@@ -638,7 +638,7 @@
 	struct gprs_ns_hdr *nsh;
 	uint16_t nsvci, nsei;
 
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 	if (!msg)
 		return -ENOMEM;
 
@@ -692,7 +692,7 @@
 		msgb_free(msg);
 		return rc;
 	}
-	log_set_context(GPRS_CTX_NSVC, nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, nsvc);
 
 	msg->l2h = msgb_push(msg, sizeof(*nsh) + 3);
 	nsh = (struct gprs_ns_hdr *) msg->l2h;
@@ -1099,7 +1099,7 @@
 		struct gprs_nsvc *fallback_nsvc;
 
 		fallback_nsvc = nsi->unknown_nsvc;
-		log_set_context(GPRS_CTX_NSVC, fallback_nsvc);
+		log_set_context(LOGGING_CTX_GB_NSVC, fallback_nsvc);
 		fallback_nsvc->ip.bts_addr = *saddr;
 		fallback_nsvc->ll = ll;
 
@@ -1215,7 +1215,7 @@
 	/* Only the RESET procedure creates a new NSVC */
 	if (nsh->pdu_type != NS_PDUT_RESET) {
 		/* Since we have no NSVC, we have to use a fake */
-		log_set_context(GPRS_CTX_NSVC, fallback_nsvc);
+		log_set_context(LOGGING_CTX_GB_NSVC, fallback_nsvc);
 		LOGP(DNS, LOGL_INFO, "Rejecting NS PDU type 0x%0x "
 		     "from %s for non-existing NS-VC\n",
 		     nsh->pdu_type, gprs_ns_ll_str(fallback_nsvc));
@@ -1256,7 +1256,7 @@
 	if (!existing_nsvc) {
 		*new_nsvc = gprs_nsvc_create(nsi, 0xffff);
 		(*new_nsvc)->nsvci_is_valid = 0;
-		log_set_context(GPRS_CTX_NSVC, *new_nsvc);
+		log_set_context(LOGGING_CTX_GB_NSVC, *new_nsvc);
 		gprs_ns_ll_copy(*new_nsvc, fallback_nsvc);
 		LOGP(DNS, LOGL_INFO, "Creating NS-VC for BSS at %s\n",
 		     gprs_ns_ll_str(fallback_nsvc));
@@ -1299,7 +1299,7 @@
 
 	msgb_nsei(msg) = (*nsvc)->nsei;
 
-	log_set_context(GPRS_CTX_NSVC, *nsvc);
+	log_set_context(LOGGING_CTX_GB_NSVC, *nsvc);
 
 	/* Increment number of Incoming bytes */
 	rate_ctr_inc(&(*nsvc)->ctrg->ctr[NS_CTR_PKTS_IN]);
diff --git a/src/gb/gprs_ns_vty.c b/src/gb/gprs_ns_vty.c
index ee305ba..2026c7a 100644
--- a/src/gb/gprs_ns_vty.c
+++ b/src/gb/gprs_ns_vty.c
@@ -61,11 +61,11 @@
 				struct gprs_nsvc *nsvc)
 {
 	if (nsvc) {
-		target->filter_map |= (1 << FLT_NSVC);
-		target->filter_data[FLT_NSVC] = nsvc;
-	} else if (target->filter_data[FLT_NSVC]) {
-		target->filter_map = ~(1 << FLT_NSVC);
-		target->filter_data[FLT_NSVC] = NULL;
+		target->filter_map |= (1 << LOGGING_FILTER_GB_NSVC);
+		target->filter_data[LOGGING_FILTER_GB_NSVC] = nsvc;
+	} else if (target->filter_data[LOGGING_FILTER_GB_NSVC]) {
+		target->filter_map = ~(1 << LOGGING_FILTER_GB_NSVC);
+		target->filter_data[LOGGING_FILTER_GB_NSVC] = NULL;
 	}
 }
 
diff --git a/src/logging.c b/src/logging.c
index 9b7d6f4..b09d684 100644
--- a/src/logging.c
+++ b/src/logging.c
@@ -48,6 +48,13 @@
 
 #include <osmocom/vty/logging.h>	/* for LOGGING_STR. */
 
+osmo_static_assert(_LOGGING_CTX_COUNT <= ARRAY_SIZE(((struct log_context*)NULL)->ctx),
+		   enum_logging_ctx_items_fit_in_struct_log_context);
+osmo_static_assert(_LOGGING_FILTER_COUNT <= ARRAY_SIZE(((struct log_target*)NULL)->filter_data),
+		   enum_logging_filters_fit_in_log_target_filter_data);
+osmo_static_assert(_LOGGING_FILTER_COUNT <= 8*sizeof(((struct log_target*)NULL)->filter_map),
+		   enum_logging_filters_fit_in_log_target_filter_map);
+
 struct log_info *osmo_log_info;
 
 static struct log_context log_context;
@@ -374,7 +381,7 @@
 	/* Apply filters here... if that becomes messy we will
 	 * need to put filters in a list and each filter will
 	 * say stop, continue, output */
-	if ((tar->filter_map & LOG_FILTER_ALL) != 0)
+	if ((tar->filter_map & (1 << LOGGING_FILTER_ALL)) != 0)
 		return 1;
 
 	if (osmo_log_info->filter_fn)
@@ -492,20 +499,20 @@
 	return 0;
 }
 
-/*! \brief Enable the \ref LOG_FILTER_ALL log filter
+/*! \brief Enable the \ref LOGGING_FILTER_ALL log filter
  *  \param[in] target Log target to be affected
  *  \param[in] all enable (1) or disable (0) the ALL filter
  *
- * When the \ref LOG_FILTER_ALL filter is enabled, all log messages will
- * be printed.  It acts as a wildcard.  Setting it to \a 1 means there
- * is no filtering.
+ * When the \ref LOGGING_FILTER_ALL filter is enabled, all log messages will be
+ * printed.  It acts as a wildcard.  Setting it to \a 1 means there is no
+ * filtering.
  */
 void log_set_all_filter(struct log_target *target, int all)
 {
 	if (all)
-		target->filter_map |= LOG_FILTER_ALL;
+		target->filter_map |= (1 << LOGGING_FILTER_ALL);
 	else
-		target->filter_map &= ~LOG_FILTER_ALL;
+		target->filter_map &= ~(1 << LOGGING_FILTER_ALL);
 }
 
 /*! \brief Enable or disable the use of colored output
diff --git a/src/vty/logging_vty.c b/src/vty/logging_vty.c
index 6004c47..30ee455 100644
--- a/src/vty/logging_vty.c
+++ b/src/vty/logging_vty.c
@@ -287,7 +287,7 @@
 	}
 
 	vty_out(vty, " Log Filter 'ALL': %s%s",
-		tgt->filter_map & LOG_FILTER_ALL ? "Enabled" : "Disabled",
+		tgt->filter_map & (1 << LOGGING_FILTER_ALL) ? "Enabled" : "Disabled",
 		VTY_NEWLINE);
 
 	/* print application specific filters */
@@ -687,7 +687,7 @@
 	}
 
 	vty_out(vty, "  logging filter all %u%s",
-		tgt->filter_map & LOG_FILTER_ALL ? 1 : 0, VTY_NEWLINE);
+		tgt->filter_map & (1 << LOGGING_FILTER_ALL) ? 1 : 0, VTY_NEWLINE);
 	/* save filters outside of libosmocore, i.e. in app code */
 	if (osmo_log_info->save_fn)
 		osmo_log_info->save_fn(vty, osmo_log_info, tgt);

-- 
To view, visit https://gerrit.osmocom.org/1844
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5c343630020f4b108099696fd96c2111614c8067
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>


More information about the gerrit-log mailing list