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