[PATCH] libosmocore[master]: wqueue: Reject messges if queue is considered full

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

Holger Freyther gerrit-no-reply at lists.osmocom.org
Fri Dec 9 10:59:45 UTC 2016


Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/1243

to look at the new patch set (#2).

wqueue: Reject messges if queue is considered full

The write queue was always meant to not queue more than the
max_length messages but the implementation never rejected a
message.

Begin to log and enforce the queue size limit, add a testcase
to verify the code and initialize except_cb as part of a fix
for that new test case.

Real applications might now run into the queue limit and drop
messages where they just queued them before. It is unfortunate
but I still think it is good to implement the routine as it was
intended. We need to review osmo_wqueue_enqueue once more to
see that no msgb is leaked.

Change-Id: I1e6aef30f3e73d4bcf2967bc49f0783aa65395ae
---
M .gitignore
M src/write_queue.c
M tests/Makefile.am
M tests/testsuite.at
A tests/write_queue/wqueue_test.c
A tests/write_queue/wqueue_test.ok
6 files changed, 103 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/43/1243/2

diff --git a/.gitignore b/.gitignore
index 90c8c85..50209b0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -95,6 +95,7 @@
 tests/gsup/gsup_test
 tests/tlv/tlv_test
 tests/fsm/fsm_test
+tests/write_queue/wqueue_test
 
 utils/osmo-arfcn
 utils/osmo-auc-gen
diff --git a/src/write_queue.c b/src/write_queue.c
index 3e488ae..c7a4320 100644
--- a/src/write_queue.c
+++ b/src/write_queue.c
@@ -1,6 +1,6 @@
 /* Generic write queue implementation */
 /*
- * (C) 2010 by Holger Hans Peter Freyther
+ * (C) 2010-2016 by Holger Hans Peter Freyther
  * (C) 2010 by On-Waves
  *
  * All Rights Reserved
@@ -23,6 +23,7 @@
 
 #include <errno.h>
 #include <osmocom/core/write_queue.h>
+#include <osmocom/core/logging.h>
 
 /*! \addtogroup write_queue
  *  @{
@@ -93,6 +94,7 @@
 	queue->current_length = 0;
 	queue->read_cb = NULL;
 	queue->write_cb = NULL;
+	queue->except_cb = NULL;
 	queue->bfd.cb = osmo_wqueue_bfd_cb;
 	INIT_LLIST_HEAD(&queue->msg_queue);
 }
@@ -104,8 +106,11 @@
  */
 int osmo_wqueue_enqueue(struct osmo_wqueue *queue, struct msgb *data)
 {
-//	if (queue->current_length + 1 >= queue->max_length)
-//		LOGP(DMSC, LOGL_ERROR, "The queue is full. Dropping not yet implemented.\n");
+	if (queue->current_length >= queue->max_length) {
+		LOGP(DLGLOBAL, LOGL_ERROR,
+			"wqueue(%p) is full. Rejecting msgb\n", queue);
+		return -ENOSPC;
+	}
 
 	++queue->current_length;
 	msgb_enqueue(&queue->msg_queue, data);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f5d095d..1aad2e9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -13,7 +13,8 @@
 		 vty/vty_test comp128/comp128_test utils/utils_test	\
 		 smscb/gsm0341_test stats/stats_test			\
 		 bitvec/bitvec_test msgb/msgb_test bits/bitcomp_test	\
-		 tlv/tlv_test gsup/gsup_test fsm/fsm_test
+		 tlv/tlv_test gsup/gsup_test fsm/fsm_test	\
+		 write_queue/wqueue_test
 
 if ENABLE_MSGFILE
 check_PROGRAMS += msgfile/msgfile_test
@@ -133,6 +134,9 @@
 fsm_fsm_test_SOURCES = fsm/fsm_test.c
 fsm_fsm_test_LDADD = $(top_builddir)/src/libosmocore.la
 
+write_queue_wqueue_test_SOURCES = write_queue/wqueue_test.c
+write_queue_wqueue_test_LDADD = $(top_builddir)/src/libosmocore.la
+
 # The `:;' works around a Bash 3.2 bug when the output is not writeable.
 $(srcdir)/package.m4: $(top_srcdir)/configure.ac
 	:;{ \
@@ -168,7 +172,7 @@
 	     utils/utils_test.ok stats/stats_test.ok			\
 	     bitvec/bitvec_test.ok msgb/msgb_test.ok bits/bitcomp_test.ok \
 	     sim/sim_test.ok tlv/tlv_test.ok gsup/gsup_test.ok		\
-	     fsm/fsm_test.ok fsm/fsm_test.err
+	     fsm/fsm_test.ok fsm/fsm_test.err write_queue/wqueue_test.ok
 
 DISTCLEANFILES = atconfig atlocal
 
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 77038bc..c01f4af 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -177,6 +177,12 @@
 AT_CHECK([$abs_top_builddir/tests/stats/stats_test], [0], [expout], [ignore])
 AT_CLEANUP
 
+AT_SETUP([write_queue])
+AT_KEYWORDS([write_queue])
+cat $abs_srcdir/write_queue/wqueue_test.ok > expout
+AT_CHECK([$abs_top_builddir/tests/write_queue/wqueue_test], [0], [expout], [ignore])
+AT_CLEANUP
+
 AT_SETUP([bssgp-fc])
 AT_KEYWORDS([bssgp-fc])
 cat $abs_srcdir/gb/bssgp_fc_tests.ok > expout
diff --git a/tests/write_queue/wqueue_test.c b/tests/write_queue/wqueue_test.c
new file mode 100644
index 0000000..827e4e8
--- /dev/null
+++ b/tests/write_queue/wqueue_test.c
@@ -0,0 +1,81 @@
+#include <osmocom/core/logging.h>
+#include <osmocom/core/utils.h>
+#include <osmocom/core/write_queue.h>
+
+static const struct log_info_cat default_categories[] = {
+};
+
+static const struct log_info log_info = {
+	.cat = default_categories,
+	.num_cat = ARRAY_SIZE(default_categories),
+};
+
+static void test_wqueue_limit(void)
+{
+	struct msgb *msg;
+	struct osmo_wqueue wqueue;
+	int rc;
+
+	osmo_wqueue_init(&wqueue, 0);
+	OSMO_ASSERT(wqueue.max_length == 0);
+	OSMO_ASSERT(wqueue.current_length == 0);
+	OSMO_ASSERT(wqueue.read_cb == NULL);
+	OSMO_ASSERT(wqueue.write_cb == NULL);
+	OSMO_ASSERT(wqueue.except_cb == NULL);
+
+	/* try to add and fail */
+	msg = msgb_alloc(4096, "msg1");
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc < 0);
+
+	/* add one and fail on the second */
+	wqueue.max_length = 1;
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc == 0);
+	OSMO_ASSERT(wqueue.current_length == 1);
+	msg = msgb_alloc(4096, "msg2");
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc < 0);
+
+	/* add one more */
+	wqueue.max_length = 2;
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc == 0);
+	OSMO_ASSERT(wqueue.current_length == 2);
+
+	/* release everything */
+	osmo_wqueue_clear(&wqueue);
+	OSMO_ASSERT(wqueue.current_length == 0);
+	OSMO_ASSERT(wqueue.max_length == 2);
+
+	/* Add two, fail on the third, free it and the queue */
+	msg = msgb_alloc(4096, "msg3");
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc == 0);
+	OSMO_ASSERT(wqueue.current_length == 1);
+	msg = msgb_alloc(4096, "msg4");
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc == 0);
+	OSMO_ASSERT(wqueue.current_length == 2);
+	msg = msgb_alloc(4096, "msg5");
+	rc = osmo_wqueue_enqueue(&wqueue, msg);
+	OSMO_ASSERT(rc < 0);
+	OSMO_ASSERT(wqueue.current_length == 2);
+	msgb_free(msg);
+	osmo_wqueue_clear(&wqueue);
+}
+
+int main(int argc, char **argv)
+{
+	struct log_target *stderr_target;
+
+	log_init(&log_info, NULL);
+	stderr_target = log_target_create_stderr();
+	log_add_target(stderr_target);
+	log_set_print_filename(stderr_target, 0);
+
+	test_wqueue_limit();
+
+	printf("Done\n");
+	return 0;
+}
diff --git a/tests/write_queue/wqueue_test.ok b/tests/write_queue/wqueue_test.ok
new file mode 100644
index 0000000..a965a70
--- /dev/null
+++ b/tests/write_queue/wqueue_test.ok
@@ -0,0 +1 @@
+Done

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e6aef30f3e73d4bcf2967bc49f0783aa65395ae
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: lynxis lazus <lynxis at fe80.eu>



More information about the gerrit-log mailing list