laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35294?usp=email )
Change subject: core: fix wrong logic in _osmo_it_q_dequeue() ......................................................................
core: fix wrong logic in _osmo_it_q_dequeue()
If the given queue is empty, queue->list.next points to &queue->list. Current implementation would call llist_del() on the queue's llist_head, decrement queue->current_length (which will be 0), and return a pointer to &queue->list to the caller. This is completely wrong.
- Use the existing item_dequeue(), which does exactly what we need. - Do not decrement the current_length if nothing was dequeued. - Uncomment code in the unit test, we should not crash anymore.
Change-Id: I63094df73b166b549616c869ad908e9f4f7d46d1 Fixes: CID#336557 --- M src/core/it_q.c M tests/it_q/it_q_test.c M tests/it_q/it_q_test.ok 3 files changed, 25 insertions(+), 11 deletions(-)
Approvals: jolly: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified
diff --git a/src/core/it_q.c b/src/core/it_q.c index a3ff420..810dc90 100644 --- a/src/core/it_q.c +++ b/src/core/it_q.c @@ -245,7 +245,7 @@
/*! Thread-safe de-queue from an inter-thread message queue. * \param[in] queue Inter-thread queue from which to dequeue - * \returns dequeued message buffer; NULL if none available + * \returns llist_head of dequeued message; NULL if none available */ struct llist_head *_osmo_it_q_dequeue(struct osmo_it_q *queue) { @@ -254,12 +254,9 @@
pthread_mutex_lock(&queue->mutex);
- if (llist_empty(&queue->list)) - l = NULL; - l = queue->list.next; - OSMO_ASSERT(l); - llist_del(l); - queue->current_length--; + l = item_dequeue(&queue->list); + if (l != NULL) + queue->current_length--;
pthread_mutex_unlock(&queue->mutex);
diff --git a/tests/it_q/it_q_test.c b/tests/it_q/it_q_test.c index 9545183..6025e39 100644 --- a/tests/it_q/it_q_test.c +++ b/tests/it_q/it_q_test.c @@ -81,11 +81,9 @@ q1 = osmo_it_q_alloc(OTC_GLOBAL, "q1", 12, NULL, NULL); OSMO_ASSERT(q1);
-#if 0 printf("try dequeueing from an empty queue\n"); osmo_it_q_dequeue(q1, &item, list); OSMO_ASSERT(item == NULL); -#endif
printf("adding queue entries up to the limit\n"); for (unsigned int i = 0; i < qlen; i++) { @@ -101,11 +99,9 @@ talloc_free(item); }
-#if 0 printf("try dequeueing from an empty queue\n"); osmo_it_q_dequeue(q1, &item, list); OSMO_ASSERT(item == NULL); -#endif
osmo_it_q_destroy(q1); } diff --git a/tests/it_q/it_q_test.ok b/tests/it_q/it_q_test.ok index 91ba0ce..e89b78d 100644 --- a/tests/it_q/it_q_test.ok +++ b/tests/it_q/it_q_test.ok @@ -11,8 +11,10 @@
== Entering test case tc_enqueue_dequeue allocating q1 +try dequeueing from an empty queue adding queue entries up to the limit removing queue entries up to the limit +try dequeueing from an empty queue
== Entering test case tc_eventfd allocating q1