diff mbox

libosmo-netif[master]: osmux: Add function to delete all msgs pending for a circuit

Message ID gerrit.1464195120088.Ib0311652183332d0475bf7347023d518d38487ef@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 25, 2016, 4:52 p.m. UTC 
Review at  https://gerrit.osmocom.org/120

osmux: Add function to delete all msgs pending for a circuit

Use this function in osmux_batch_del_circuit() since msgs are stored in a list
per circuit. After the circuit is free()d the msgs are lost.
Before this patch any messages enqueued inside a batch when the circiut is
deleted were leaking.

Change-Id: Ib0311652183332d0475bf7347023d518d38487ef
Ticket: OS#1733
---
M src/osmux.c
1 file changed, 12 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/20/120/1

Comments

gerrit-no-reply@lists.osmocom.org May 25, 2016, 7:28 p.m. UTC | #1 
Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/120/1/src/osmux.c
File src/osmux.c:

Line 236: 		}
good. but indention is wrong. :)
gerrit-no-reply@lists.osmocom.org May 25, 2016, 8:57 p.m. UTC | #2 
Patch Set 2:

(5 comments)

Okay, we can go through that code in terms of reliability. But you seem to fix it. The timer question is not that important.

https://gerrit.osmocom.org/#/c/120/2/src/osmux.c
File src/osmux.c:

PS2, Line 234: batch
so what if nmsgs == 0? Will we need to stop the batch timer? What if the batch timer fires and there is no single msgb? Did you review the code in that regard?


Line 325: 				      uint32_t batch_size, uint32_t batch_factor)
How does it use the batch_factor? We seem to consume everything? Besides for the dummy handling? Does it mean we put dummy + data into the same message?


Line 337: 		return NULL;
the caller will crash if that happens


PS2, Line 369: 				
Leaks the batch_msg. And discards other frames.. already formatted into the patch.. The caller will crash.


Line 399: 	h->stats.output_osmux_bytes += batch_msg->len;
Unconditional access of batch_msg that might be NULL
gerrit-no-reply@lists.osmocom.org May 26, 2016, 12:05 p.m. UTC | #3 
Patch Set 2: Code-Review+2

Please update the tickets with the other things I found during review.
diff mbox

Patch

diff --git a/src/osmux.c b/src/osmux.c
index 44b4b96..01d0bdc 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -225,6 +225,17 @@ 
 	circuit->nmsgs--;
 }
 
+static void osmux_circuit_del_msgs(struct osmux_batch *batch, struct osmux_circuit *circuit)
+{
+	struct msgb *cur, *tmp;
+		llist_for_each_entry_safe(cur, tmp, &circuit->msg_list, list) {
+
+			osmux_batch_dequeue(cur, circuit);
+			msgb_free(cur);
+			batch->nmsgs--;
+		}
+}
+
 struct osmux_input_state {
 	struct msgb	*out_msg;
 	struct msgb	*msg;
@@ -538,6 +549,7 @@ 
 	if (circuit->dummy)
 		batch->ndummy--;
 	llist_del(&circuit->head);
+	osmux_circuit_del_msgs(batch, circuit);
 	talloc_free(circuit);
 }