Attention is currently required from: laforge, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34900?usp=email )
Change subject: add fmtp string to ptmap: allow all possible fmtp
......................................................................
Patch Set 10:
(3 comments)
File include/osmocom/mgcp/fmtp.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/09d33fc1_163b5854
PS10, Line 1: #pragma once
> libosmocore is too generic for mgcp specific stuff. […]
ok, will try that
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/8e78d209_ad654820
PS6, Line 1372: !
> ==
(somehow gerrit lost the right place for this. the == is below.)
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/3ab13f10_31029c60
PS9, Line 1290: && (mgcp_msg->ptmap[i].codec == CODEC_AMR_8000_1
> (fact: the problem with the "&&" at the start of the line instead of appending at the end of last on […]
- it's much more efficient to read the condition with operators at the start.
at the start, they are in a fixed position, and form a tree structure (akin to a file tree view) with the operators as the node bullets.
With operators at the end, the eye needs to scan around.
- the loss of indent happens only once per logical branch in the condition and is only 3 chars...
i would very much like to continue using this structuring in my conditions.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34900?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: If58590bda8627519ff07e0b6f43aa47a274f052b
Gerrit-Change-Number: 34900
Gerrit-PatchSet: 10
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: dexter <pmaier(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 13 Dec 2023 01:46:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/35348?usp=email )
Change subject: ipa: Use ASP name as ipa_unit_name on dynamic ASPs
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/35348?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Change-Id: I0a741449450c998253b1e44a76a3b7fc224e0903
Gerrit-Change-Number: 35348
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 12 Dec 2023 23:32:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35349?usp=email )
Change subject: client: safely handle dealloc on event dispatch
......................................................................
client: safely handle dealloc on event dispatch
See also the long in-code comment.
Related: OS#6302
Change-Id: I6f1c0f6a26f9cd6993dc1910a44070ec0438e636
---
M src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c
1 file changed, 46 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/49/35349/1
diff --git a/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c b/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c
index 105e54b..6fbfa4d 100644
--- a/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c
+++ b/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c
@@ -533,10 +533,42 @@
mgcp_conn_peer_name(ci->got_port_info? &ci->rtp_info : NULL),
ci->notify.fi ? "" : " (not sending a notification)");
+ /* Below ordering is a delicate decision:
+ *
+ * We want to
+ * - emit the resulting event to ci->notify.fi,
+ * - check whether we want to tx the next pending MGCP message.
+ * Both these steps may terminate (=deallocate) the ep.
+ * So whichever one goes first may cause a use-after-free in the other.
+ *
+ * When dispatching the FSM event, we don't get an rc indicating dealloc of the FSM -- it may deallocate and we
+ * cannot tell. The common mechanism for that is osmo_fsm_set_dealloc_ctx(OTC_SELECT) and query the still
+ * allocated FSM state after termination (here we would check 'if (ci->ep != NULL)'), but we cannot assume the
+ * caller has actually set up an osmo_fsm_set_dealloc_ctx(). At time of writing, e.g. osmo-hnbgw does not use
+ * it.
+ *
+ * In osmo_mgcpc_ep_fsm_check_state_chg_after_response(), we do get an rc: false means FSM has terminated.
+ * On termination, the ep emits a term event to the FSM's parent.
+ * That may cause the notify.fi to be terminated in turn, depending on how the caller set things up.
+ * So: we cannot store notify.fi before, then call osmo_mgcpc_ep_fsm_check_state_chg_after_response(), and then
+ * emit the event, because notify.fi may have deallocated. We cannot look up whether
+ * osmo_mgcpc_ep_cancel_notify() has been called, because ci may have deallocated along with ci->ep.
+ *
+ * We have to skip emitting below success event in case the ep is now terminated.
+ * - It may be the final DLCX OK: not a problem, osmo_mgcpc_ep_ci_dlcx() has no notify args on purpose, so we do
+ * make all callers not set a notify event for DLCX by design. notify.fi should always be NULL when the final
+ * DLCX OK terminates the local endpoint state.
+ * - It may also be sudden termination due to a bad problem, in which case we shouldn't emit success.
+ * The osmo_fsm_inst.parent_term_event should suffice as feedback to the caller.
+ */
+
+ if (osmo_mgcpc_ep_fsm_check_state_chg_after_response(ci->ep->fi) == false) {
+ /* false means, the ci->ep has been terminated. */
+ return;
+ }
+
if (ci->notify.fi)
osmo_fsm_inst_dispatch(ci->notify.fi, ci->notify.success, ci->notify.data);
-
- osmo_mgcpc_ep_fsm_check_state_chg_after_response(ci->ep->fi);
}
/*! Return the MGW's local RTP port information for this connection, i.e. the local port that MGW is receiving on, as
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35349?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6f1c0f6a26f9cd6993dc1910a44070ec0438e636
Gerrit-Change-Number: 35349
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: laforge, pespin.
Hello Jenkins Builder, laforge,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-sccp/+/35348?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review-1 by pespin, Verified+1 by Jenkins Builder
Change subject: ipa: Use ASP name as ipa_unit_name on dynamic ASPs
......................................................................
ipa: Use ASP name as ipa_unit_name on dynamic ASPs
A recent patch fixed a long problem where the ASP name (instead of
expected AS name) was used as ipa_unit_name in IPA based ASPs.
For server side it doesn't matter much, sense anyway the ipa_unit_name
is actually restored on the struct with what's received in IPA GET_RESP
message (see ipa_asp_fsm_wait_id_resp()). So the fix was actually for
the client side in the scenario where a non-dynamic ASP with an assigned
AS was configured in the VTY.
However, dynamic ASPs usually have no assigned AS (because in general it
is really not created/configured, as the ASP is created on the flight).
As a result, the recent patch (see "Fixes" below), broke dynamic ASPs
case because from then one ipa_asp_fsm_start() would fail and terminate
the FSM because ipa_find_as_for_asp() was returning NULL.
So improve the recent patch by applying the previous logic for dynamic
ASPs:
* On the server side, it really doesn't matter since as mentioned, the
field will be repopulated later on, but allows the code to avoid
terminating the FSM and hence be brought up and be ready to receive
clients.
* On the client case, this is how dynamic IPA ASPs were ment to be used
when they were introduced anyway (use ASP as ipa_unit_id, meaning
"AS name" == "ASP name").
Furthermore, on the client side, the non-dynamic IPA ASPs need their
bring up be delayed until assigned to an AS, because the AS name is sent
in ipa_unit_name field in Tx IPA ID RESP.
This usually happens at a later point than ASP (FSM) creation, because
first the ASP object is created (through VTY or API) and then assigned
to an AS through osmo_ss7_as_add_asp() usually from a later "asp" vty
command in the "as" node.
Fixes: 65741dca056e3a16973ad156dd4c09760a6a945b
Change-Id: I0a741449450c998253b1e44a76a3b7fc224e0903
Related: SYS#5914
---
M src/osmo_ss7_as.c
M src/xua_asp_fsm.c
M src/xua_asp_fsm.h
3 files changed, 86 insertions(+), 8 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/48/35348/2
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/35348?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Change-Id: I0a741449450c998253b1e44a76a3b7fc224e0903
Gerrit-Change-Number: 35348
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/35348?usp=email )
Change subject: ipa: Use ASP name as ipa_unit_name on dynamic ASPs
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Patchset:
PS1:
This is still not fixing the entire problem, because the ASP FSM is started during ASP creation in VTY, that means before the AS is created and VTY and hence gets assigned to ASP...
We should ideally delay ASP UP until the whole config is read.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/35348?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Change-Id: I0a741449450c998253b1e44a76a3b7fc224e0903
Gerrit-Change-Number: 35348
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Tue, 12 Dec 2023 17:21:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment