[PATCH] libosmo-netif[master]: osmux: Add function to delete all msgs pending for a circuit
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 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); } -- To view, visit https://gerrit.osmocom.org/120 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib0311652183332d0475bf7347023d518d38487ef Gerrit-PatchSet: 1 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: daniel <dwillmann@sysmocom.de>
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. :) -- To view, visit https://gerrit.osmocom.org/120 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0311652183332d0475bf7347023d518d38487ef Gerrit-PatchSet: 1 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: daniel <dwillmann@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/120 to look at the new patch set (#2). 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, 11 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/20/120/2 diff --git a/src/osmux.c b/src/osmux.c index 1f5bbe2..0bee9cc 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -225,6 +225,16 @@ 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 +548,7 @@ if (circuit->dummy) batch->ndummy--; llist_del(&circuit->head); + osmux_circuit_del_msgs(batch, circuit); talloc_free(circuit); } -- To view, visit https://gerrit.osmocom.org/120 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib0311652183332d0475bf7347023d518d38487ef Gerrit-PatchSet: 2 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: daniel <dwillmann@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder
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 -- To view, visit https://gerrit.osmocom.org/120 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0311652183332d0475bf7347023d518d38487ef Gerrit-PatchSet: 2 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: daniel <dwillmann@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
Patch Set 2: Code-Review+2 Please update the tickets with the other things I found during review. -- To view, visit https://gerrit.osmocom.org/120 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0311652183332d0475bf7347023d518d38487ef Gerrit-PatchSet: 2 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: daniel <dwillmann@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: No
Holger Freyther has submitted this change and it was merged. Change subject: osmux: Add function to delete all msgs pending for a circuit ...................................................................... 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 Reviewed-on: https://gerrit.osmocom.org/120 Tested-by: Jenkins Builder Reviewed-by: Holger Freyther <holger@freyther.de> --- M src/osmux.c 1 file changed, 11 insertions(+), 0 deletions(-) Approvals: Jenkins Builder: Verified Holger Freyther: Looks good to me, approved diff --git a/src/osmux.c b/src/osmux.c index 1f5bbe2..0bee9cc 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -225,6 +225,16 @@ 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 +548,7 @@ if (circuit->dummy) batch->ndummy--; llist_del(&circuit->head); + osmux_circuit_del_msgs(batch, circuit); talloc_free(circuit); } -- To view, visit https://gerrit.osmocom.org/120 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib0311652183332d0475bf7347023d518d38487ef Gerrit-PatchSet: 3 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: daniel <dwillmann@sysmocom.de> Gerrit-Reviewer: Holger Freyther <holger@freyther.de> Gerrit-Reviewer: Jenkins Builder
participants (2)
-
daniel -
Holger Freyther