January 2024 - gerrit-log - lists.osmocom.org

[M] Change in osmo-mgw[master]: add fmtp.[hc]
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: laforge, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35434?usp=email ) Change subject: add fmtp.[hc] ...................................................................... Patch Set 5: (1 comment) Patchset: PS3: > The intention was to only do fmtp specific things, but i feel a tendency here to absorb also other p […] ...as a new libosmo-sdp.so that both osmo-mgw and MGCP clients can link? -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35434?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: I3eaea353dbd98c19212199b564342d0ac16cbc07 Gerrit-Change-Number: 35434 Gerrit-PatchSet: 5 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Fri, 05 Jan 2024 02:14:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-mgw[master]: add fmtp.[hc]
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: laforge, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35434?usp=email ) Change subject: add fmtp.[hc] ...................................................................... Patch Set 5: (6 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/e32e3e03_dd185761 PS4, Line 9: Upcoming patches will add handling of arbitrary ftmp strings to > fmtp strings lol! thx https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/e6755321_69675d81 PS4, Line 13: Add generic API for handling ftmp strings. The primary intended user is > fmtp Done Patchset: PS4: > minor nitpick: the subject states "ftmp.h" while in reality it's more than a header. ah yes, it already secretly morphed from just-a-header to a fmtp.c. Now it wants to morph into a libosmo-sdp.so. File src/libosmo-mgcp-client/fmtp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/bbd958dd_9970414c PS4, Line 29: for (; fmtp && *fmtp && *fmtp != ';'; fmtp++); > fmtp being nonull should be checked only once at start. Done https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/73bc9c47_285cf0a4 PS4, Line 41: * if (osmo_sdp_fmtp_get_val(res, sizeof(res), fmtp_vals, "mode-set")) > IIUC res contains "0,2,4,7" here, maybe worth saying that. Done https://gerrit.osmocom.org/c/osmo-mgw/+/35434/comment/be198855_4b453326 PS4, Line 89: int osmo_sdp_fmtp_get_int(const char *fmtp, const char *option_name, int default_value) > did you check in SDP spec if int range is enough? the fmtp contents are codec specific and not defined in SDP, apart from their general structure (which characters to use for separation). For our known fmtp use cases, we will use numbers as high as 7. If a fmtp needs a number above two billion, then the caller should use osmo_sdp_fmtp_get_val() to retrieve the value as string. This convenience function is useful only for very very small integers, i.e. below ... TWO BILLION =) scnr But it took me longer to type this response than it would take to increase the range, so i'm also increasing the range now. -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35434?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: I3eaea353dbd98c19212199b564342d0ac16cbc07 Gerrit-Change-Number: 35434 Gerrit-PatchSet: 5 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Fri, 05 Jan 2024 02:12:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-mgw[master]: add fmtp.[hc]
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: laforge, neels. Hello Jenkins Builder, laforge, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/35434?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Code-Review+1 by laforge, Verified+1 by Jenkins Builder Change subject: add fmtp.[hc] ...................................................................... add fmtp.[hc] Upcoming patches will add handling of arbitrary fmtp strings to osmo-mgw as well as libosmo-mgcp-client (If58590bda8627519ff07e0b6f43aa47a274f052b). Add generic API for handling fmtp strings. The primary intended user is osmo-mgw, but this is also generally useful to libosmo-mgcp-client callers for parsing the received fmtp. We discussed that fmtp.[hc] should be published in libosmo-mgcp-client, but also built within osmo-mgw. Hence: - put fmtp.h and .c in libosmo-mgcp-client/ - in osmo-mgw code, use #include <mgcp_client/fmtp.h> and build the src/libosmo-mgcp-client/fmtp.c also into libosmo-mgcp.a. (This is currently the quickest solution and without side effects since fmtp.c is fully self-contained; an alternative would be adding a not-installed libmgcp-common.a within the build tree) Change-Id: I3eaea353dbd98c19212199b564342d0ac16cbc07 --- M include/Makefile.am A include/osmocom/mgcp_client/fmtp.h M src/libosmo-mgcp-client/Makefile.am A src/libosmo-mgcp-client/fmtp.c M src/libosmo-mgcp/Makefile.am 5 files changed, 190 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/34/35434/5 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35434?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: I3eaea353dbd98c19212199b564342d0ac16cbc07 Gerrit-Change-Number: 35434 Gerrit-PatchSet: 5 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-mgw[master]: drop (now) unused code
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: laforge, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35247?usp=email ) Change subject: drop (now) unused code ...................................................................... Patch Set 6: Code-Review+2 (1 comment) Patchset: PS6: re-add votes after adding a TODO-RELEASE entry -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35247?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: I84e5285831397c992af59deee12dea8458d16cc6 Gerrit-Change-Number: 35247 Gerrit-PatchSet: 6 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-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Fri, 05 Jan 2024 02:07:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in osmo-mgw[master]: mgcp_client_test: add test_parse_response()
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: neels. Hello Jenkins Builder, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/35422?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder The change is no longer submittable: Verified is unsatisfied now. Change subject: mgcp_client_test: add test_parse_response() ...................................................................... mgcp_client_test: add test_parse_response() Change-Id: I842ce65a9a70f313570857b7df53727cc572b9e6 --- M tests/mgcp_client/mgcp_client_test.c M tests/mgcp_client/mgcp_client_test.err 2 files changed, 266 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/22/35422/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35422?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: I842ce65a9a70f313570857b7df53727cc572b9e6 Gerrit-Change-Number: 35422 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-mgw[master]: drop (now) unused code
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: laforge, neels, pespin. Hello Jenkins Builder, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/35247?usp=email to look at the new patch set (#6). The following approvals got outdated and were removed: Code-Review+1 by pespin, Code-Review+2 by laforge, Verified+1 by Jenkins Builder The change is no longer submittable: Code-Review and Verified are unsatisfied now. Change subject: drop (now) unused code ...................................................................... drop (now) unused code Removing the duality of codecs[] and ptmap[] in structs mgcp_msg, mgcp_response and mgcp_conn_peer has removed the need to "map" from codec type enum to payload type number. They are stored together now. Remove functions that are no longer used. None of our osmocom users of libosmo-mgcp-client call these functions. Change-Id: I84e5285831397c992af59deee12dea8458d16cc6 --- M TODO-RELEASE M include/osmocom/mgcp_client/mgcp_client.h M src/libosmo-mgcp-client/mgcp_client.c M tests/mgcp_client/mgcp_client_test.c M tests/mgcp_client/mgcp_client_test.err M tests/mgcp_client/mgcp_client_test.ok 6 files changed, 21 insertions(+), 177 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/47/35247/6 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35247?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: I84e5285831397c992af59deee12dea8458d16cc6 Gerrit-Change-Number: 35247 Gerrit-PatchSet: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-mgw[master]: client: allow MGCP_MAX_CODECS entries
by neels 05 Jan '24

05 Jan '24

neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35420?usp=email ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: client: allow MGCP_MAX_CODECS entries ...................................................................... client: allow MGCP_MAX_CODECS entries So far we allow only MGCP_MAX_CODECS-1 entries, because the parsing exit condition hits only after the array size check. Instead, check the array size a bit later, just before actually adding a valid entry. This is verified to work as expected in upcoming patch I842ce65a9a70f313570857b7df53727cc572b9e6 that adds a new mgcp_client_test.c section for this. Change-Id: I9a28da85e437f118026ea71a5a708e5758fff623 --- M src/libosmo-mgcp-client/mgcp_client.c 1 file changed, 21 insertions(+), 4 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/libosmo-mgcp-client/mgcp_client.c b/src/libosmo-mgcp-client/mgcp_client.c index 60c54a6..8df65cd 100644 --- a/src/libosmo-mgcp-client/mgcp_client.c +++ b/src/libosmo-mgcp-client/mgcp_client.c @@ -323,10 +323,6 @@ pt_str = strtok(line, " "); while (1) { - /* Do not allow excessive payload types */ - if (ptmap_len >= ARRAY_SIZE(r->ptmap)) - goto response_parse_failure_pt; - pt_str = strtok(NULL, " "); if (!pt_str) break; @@ -344,6 +340,10 @@ if (r->ptmap[i].pt == pt) goto response_parse_failure_pt; + /* Do not allow excessive payload types */ + if (ptmap_len >= ARRAY_SIZE(r->ptmap)) + goto response_parse_failure_pt; + /* Some payload type numbers imply a specific codec. For those, using the PT number as enum mgcp_codecs * yields the correct result. If no more specific information on the codec follows in "a=rtpmap:N" * lines, then this default number takes over. This only applies for PT below the dynamic range (<96). */ -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35420?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: I9a28da85e437f118026ea71a5a708e5758fff623 Gerrit-Change-Number: 35420 Gerrit-PatchSet: 2 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-MessageType: merged

1 0

[L] Change in osmo-mgw[master]: client: collapse codecs[] and ptmap[]; allow codec variants
by neels 05 Jan '24

05 Jan '24

neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34899?usp=email ) Change subject: client: collapse codecs[] and ptmap[]; allow codec variants ...................................................................... client: collapse codecs[] and ptmap[]; allow codec variants codecs[] is an array of enum osmo_mgcp_codecs. ptmap[] is an array of { enum osmo_mgcp_codecs, unsigned int ptmap }. MGCP lists first a bunch of payload type numbers and then specifies them again for details, like the numbers 112, 96, 3 in this example: m=audio <port> RTP/AVP 112 96 3 a=rtpmap:112 AMR/8000 a=rtpmap:96 VND.3GPP.IUFP/16000 a=rtpmap:3 GSM-FR/8000 So far we keep these lists in two separate arrays: - codecs[], codecs_len stores the 'm=audio' list - ptmap[], ptmap_len stores the 'a=rtpmap' list (and may omit some elements present in codecs[]) This applies to both struct mgcp_response and struct mgcp_msg. These are semantically identical, and the separation leads to checks, conversions and dear hopes of correct ordering. So let's keep only one list with both codec and payload type number in it. The 'm=audio' list establishes the order of the pt numbers, and the 'a=rtpmap' list adds codec information to the established entries. In the internal API structs mgcp_msg and mgcp_response, just drop the codecs[] entirely. In public API struct mgcp_conn_peer, keep the codecs[] array, mark it deprecated, and provide a backwards compat conversion: if any caller invokes mgcp_conn_create() or mgcp_conn_modify() with codecs[] still present in the verb_info arg, then move codecs[] entries over to the ptmap[] array in a sensible way. (BTW, even mgcp_conn_create() and mgcp_conn_modify() are never called from outside of libosmo-mgcp-client in any of our osmo-cni programs; users call osmo_mgcpc_ep_ci_add() and osmo_mgcpc_ep_ci_request(), which in turn may pass user-provided codecs[] lists on to mgcp_conn_create() or mgcp_conn_modify().) Tests for parsing the MGCP response are mostly missing. They will be added in upcoming patch I842ce65a9a70f313570857b7df53727cc572b9e6, because they will be using only the new ptmap API. Related: OS#6171 Change-Id: I798e02c6663376d3d52f4a74fc4b32411ce95bed --- M TODO-RELEASE M include/osmocom/mgcp_client/mgcp_client.h M include/osmocom/mgcp_client/mgcp_client_fsm.h M include/osmocom/mgcp_client/mgcp_client_internal.h M src/libosmo-mgcp-client/mgcp_client.c M src/libosmo-mgcp-client/mgcp_client_fsm.c M tests/mgcp_client/mgcp_client_test.c 7 files changed, 227 insertions(+), 74 deletions(-) Approvals: neels: Looks good to me, approved Jenkins Builder: Verified diff --git a/TODO-RELEASE b/TODO-RELEASE index 82368ff..964ffe1 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -35,3 +35,6 @@ mgcp_client_cancel() mgcp_msg_gen() mgcp_msg_trans_id() +libosmo-mgcp-client deprecate public API New code should no longer use codecs[], instead use ptmap[].codec. There + is backwards compat code that moves codecs[] entries, if any, over to + ptmap[], so callers may migrate at own leisure. diff --git a/include/osmocom/mgcp_client/mgcp_client.h b/include/osmocom/mgcp_client/mgcp_client.h index a96ae19..1d33690 100644 --- a/include/osmocom/mgcp_client/mgcp_client.h +++ b/include/osmocom/mgcp_client/mgcp_client.h @@ -79,6 +79,8 @@ unsigned int pt; }; +int ptmap_cmp(const struct ptmap *a, const struct ptmap *b); + enum mgcp_verb { MGCP_VERB_CRCX, MGCP_VERB_MDCX, diff --git a/include/osmocom/mgcp_client/mgcp_client_fsm.h b/include/osmocom/mgcp_client/mgcp_client_fsm.h index 4e9ba89..dbd5128 100644 --- a/include/osmocom/mgcp_client/mgcp_client_fsm.h +++ b/include/osmocom/mgcp_client/mgcp_client_fsm.h @@ -29,11 +29,11 @@ /*! RTP packetization interval (optional) */ unsigned int ptime; - /*! RTP codec list (optional) */ - enum mgcp_codecs codecs[MGCP_MAX_CODECS]; - - /*! Number of codecs in RTP codec list (optional) */ - unsigned int codecs_len; + /*! Deprecated. Use only ptmap[].codec in new code. */ + enum mgcp_codecs codecs[MGCP_MAX_CODECS] + OSMO_DEPRECATED_OUTSIDE_LIBOSMOMGCPCLIENT("use ptmap[i].codec instead"); + unsigned int codecs_len + OSMO_DEPRECATED_OUTSIDE_LIBOSMOMGCPCLIENT("use ptmap[] and ptmap_len instead"); /*! RTP payload type map (optional, only needed when payload types are * used that differ from what IANA/3GPP defines) */ diff --git a/include/osmocom/mgcp_client/mgcp_client_internal.h b/include/osmocom/mgcp_client/mgcp_client_internal.h index c3619bb..423c00e 100644 --- a/include/osmocom/mgcp_client/mgcp_client_internal.h +++ b/include/osmocom/mgcp_client/mgcp_client_internal.h @@ -38,8 +38,6 @@ uint16_t audio_port; char audio_ip[INET6_ADDRSTRLEN]; unsigned int ptime; - enum mgcp_codecs codecs[MGCP_MAX_CODECS]; - unsigned int codecs_len; struct ptmap ptmap[MGCP_MAX_CODECS]; unsigned int ptmap_len; }; @@ -84,8 +82,6 @@ char *audio_ip; enum mgcp_connection_mode conn_mode; unsigned int ptime; - enum mgcp_codecs codecs[MGCP_MAX_CODECS]; - unsigned int codecs_len; struct ptmap ptmap[MGCP_MAX_CODECS]; unsigned int ptmap_len; uint32_t x_osmo_ign; diff --git a/src/libosmo-mgcp-client/mgcp_client.c b/src/libosmo-mgcp-client/mgcp_client.c index cd73391..60c54a6 100644 --- a/src/libosmo-mgcp-client/mgcp_client.c +++ b/src/libosmo-mgcp-client/mgcp_client.c @@ -302,7 +302,7 @@ char *pt_str; char *pt_end; unsigned long int pt; - unsigned int count = 0; + unsigned int ptmap_len; unsigned int i; /* Extract port information */ @@ -316,10 +316,15 @@ if (!line) goto exit; + /* Clear any previous entries before writing over r->ptmap */ + r->ptmap_len = 0; + /* Keep a local ptmap_len to show only the full list after parsing succeeded in whole. */ + ptmap_len = 0; + pt_str = strtok(line, " "); while (1) { /* Do not allow excessive payload types */ - if (count >= ARRAY_SIZE(r->codecs)) + if (ptmap_len >= ARRAY_SIZE(r->ptmap)) goto response_parse_failure_pt; pt_str = strtok(NULL, " "); @@ -335,21 +340,23 @@ goto response_parse_failure_pt; /* Do not allow duplicate payload types */ - for (i = 0; i < count; i++) - if (r->codecs[i] == pt) + for (i = 0; i < ptmap_len; i++) + if (r->ptmap[i].pt == pt) goto response_parse_failure_pt; - /* Note: The payload type we store may not necessarly match - * the codec types we have defined in enum mgcp_codecs. To - * ensure that the end result only contains codec types which - * match enum mgcp_codecs, we will go through afterwards and - * remap the affected entries with the inrofmation we learn - * from rtpmap */ - r->codecs[count] = pt; - count++; + /* Some payload type numbers imply a specific codec. For those, using the PT number as enum mgcp_codecs + * yields the correct result. If no more specific information on the codec follows in "a=rtpmap:N" + * lines, then this default number takes over. This only applies for PT below the dynamic range (<96). */ + if (pt < 96) + r->ptmap[ptmap_len].codec = pt; + else + r->ptmap[ptmap_len].codec = -1; + r->ptmap[ptmap_len].pt = pt; + ptmap_len++; } - r->codecs_len = count; + /* Parsing succeeded, publish all entries. */ + r->ptmap_len = ptmap_len; exit: return 0; @@ -365,10 +372,11 @@ return -EINVAL; } -/* Parse a line like "m=audio 16002 RTP/AVP 98", extract port and payload types */ +/* Parse an 'a=...' parameter */ static int mgcp_parse_audio_ptime_rtpmap(struct mgcp_response *r, const char *line) { unsigned int pt; + unsigned int i; char codec_resp[64]; int rc; @@ -387,18 +395,39 @@ "Failed to parse SDP parameter, invalid rtpmap: %s\n", osmo_quote_str(line, -1)); return -EINVAL; } - if (r->ptmap_len >= ARRAY_SIZE(r->ptmap)) { - LOGP(DLMGCP, LOGL_ERROR, "No more space in ptmap array (len=%u)\n", r->ptmap_len); - return -ENOSPC; - } rc = map_str_to_codec(codec_resp); if (rc < 0) { LOGP(DLMGCP, LOGL_ERROR, "Failed to parse SDP parameter, can't parse codec in rtpmap: %s\n", osmo_quote_str(line, -1)); return -EINVAL; } - r->ptmap[r->ptmap_len].pt = pt; - r->ptmap[r->ptmap_len].codec = rc; + + /* Earlier, a line like "m=audio 16002 RTP/AVP 98 112 3" established the desired order of payloads, now + * enrich it with actual codec information provided by "a=rtpmap:..." entries. + * For each, find the entry with the right pt number and add the info there. */ + + for (i = 0; i < r->ptmap_len; i++) { + if (r->ptmap[i].pt != pt) + continue; + r->ptmap[i].codec = rc; + return 0; + } + + /* No entry was found. This is an error in the MGCP protocol, but let's just add another entry + * anyway, to not make it look like it was never there. */ + LOGP(DLMGCP, LOGL_ERROR, + "error in MGCP message: 'a=rtpmap:%u' has no matching entry in 'm=audio ... %u'\n", + pt, pt); + if (r->ptmap_len >= ARRAY_SIZE(r->ptmap)) { + LOGP(DLMGCP, LOGL_ERROR, + "cannot parse all codecs: can only store up to %zu rtpmap entries.\n", + ARRAY_SIZE(r->ptmap)); + return -ENOSPC; + } + r->ptmap[r->ptmap_len] = (struct ptmap){ + .pt = pt, + .codec = rc, + }; r->ptmap_len++; } @@ -508,7 +537,6 @@ int rc; char *data; char *data_ptr; - int i; /* Since this functions performs a destructive parsing, we create a * local copy of the body data */ @@ -553,10 +581,6 @@ } } - /* See also note in mgcp_parse_audio_port_pt() */ - for (i = 0; i < r->codecs_len; i++) - r->codecs[i] = map_pt_to_codec(r->ptmap, r->ptmap_len, r->codecs[i]); - rc = 0; exit: talloc_free(data); @@ -1234,7 +1258,6 @@ { unsigned int i; const char *codec; - unsigned int pt; #define MSGB_PRINTF_OR_RET(FMT, ARGS...) do { \ if (msgb_printf(msg, FMT, ##ARGS) != 0) { \ @@ -1248,11 +1271,10 @@ if (mgcp_msg->ptime) MSGB_PRINTF_OR_RET(" p:%u,", mgcp_msg->ptime); - if (mgcp_msg->codecs_len) { + if (mgcp_msg->ptmap_len) { MSGB_PRINTF_OR_RET(" a:"); - for (i = 0; i < mgcp_msg->codecs_len; i++) { - pt = mgcp_msg->codecs[i]; - codec = get_value_string_or_null(osmo_mgcpc_codec_names, pt); + for (i = 0; i < mgcp_msg->ptmap_len; i++) { + codec = get_value_string_or_null(osmo_mgcpc_codec_names, mgcp_msg->ptmap[i].codec); /* Note: Use codec descriptors from enum mgcp_codecs * in mgcp_client only! */ @@ -1260,7 +1282,7 @@ return -EINVAL; MSGB_PRINTF_OR_RET("%s", extract_codec_name(codec)); - if (i < mgcp_msg->codecs_len - 1) + if (i < mgcp_msg->ptmap_len - 1) MSGB_PRINTF_OR_RET(";"); } MSGB_PRINTF_OR_RET(","); @@ -1338,21 +1360,19 @@ return -EINVAL; } MSGB_PRINTF_OR_RET("m=audio %u RTP/AVP", mgcp_msg->audio_port); - for (i = 0; i < mgcp_msg->codecs_len; i++) { - pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); - MSGB_PRINTF_OR_RET(" %u", pt); - - } + for (i = 0; i < mgcp_msg->ptmap_len; i++) + MSGB_PRINTF_OR_RET(" %u", mgcp_msg->ptmap[i].pt); MSGB_PRINTF_OR_RET("\r\n"); } /* Add optional codec parameters (fmtp) */ if (mgcp_msg->param_present) { - for (i = 0; i < mgcp_msg->codecs_len; i++) { + for (i = 0; i < mgcp_msg->ptmap_len; i++) { /* The following is only applicable for AMR */ - if (mgcp_msg->codecs[i] != CODEC_AMR_8000_1 && mgcp_msg->codecs[i] != CODEC_AMRWB_16000_1) + if (mgcp_msg->ptmap[i].codec != CODEC_AMR_8000_1 + && mgcp_msg->ptmap[i].codec != CODEC_AMRWB_16000_1) continue; - pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); + pt = mgcp_msg->ptmap[i].pt; if (mgcp_msg->param.amr_octet_aligned_present && mgcp_msg->param.amr_octet_aligned) MSGB_PRINTF_OR_RET("a=fmtp:%u octet-align=1\r\n", pt); else if (mgcp_msg->param.amr_octet_aligned_present && !mgcp_msg->param.amr_octet_aligned) @@ -1360,14 +1380,14 @@ } } - for (i = 0; i < mgcp_msg->codecs_len; i++) { - pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); + for (i = 0; i < mgcp_msg->ptmap_len; i++) { + pt = mgcp_msg->ptmap[i].pt; /* Note: Only dynamic payload type from the range 96-127 * require to be explained further via rtpmap. All others * are implcitly definedby the number in m=audio */ if (pt >= 96 && pt <= 127) { - codec = get_value_string_or_null(osmo_mgcpc_codec_names, mgcp_msg->codecs[i]); + codec = get_value_string_or_null(osmo_mgcpc_codec_names, mgcp_msg->ptmap[i].codec); /* Note: Use codec descriptors from enum mgcp_codecs * in mgcp_client only! */ @@ -1575,3 +1595,20 @@ else return mgcp_client_endpoint_domain(mgcp); } + +/*! Return typical cmp result, comparing a to b. + * Return 0 if a == b, -1 if a < b, 1 if a > b; comparing all members of ptmap in turn. */ +int ptmap_cmp(const struct ptmap *a, const struct ptmap *b) +{ + int rc; + if (a == b) + return 0; + if (!a) + return -1; + if (!b) + return 1; + rc = OSMO_CMP(a->codec, b->codec); + if (rc) + return rc; + return OSMO_CMP(a->pt, b->pt); +} diff --git a/src/libosmo-mgcp-client/mgcp_client_fsm.c b/src/libosmo-mgcp-client/mgcp_client_fsm.c index 43f0f50..e638d1e 100644 --- a/src/libosmo-mgcp-client/mgcp_client_fsm.c +++ b/src/libosmo-mgcp-client/mgcp_client_fsm.c @@ -115,12 +115,10 @@ .call_id = info->call_id, .conn_mode = MGCP_CONN_RECV_ONLY, .ptime = info->ptime, - .codecs_len = info->codecs_len, .ptmap_len = info->ptmap_len, .param_present = info->param_present }; osmo_strlcpy(mgcp_msg->endpoint, info->endpoint, MGCP_ENDPOINT_MAXLEN); - memcpy(mgcp_msg->codecs, info->codecs, sizeof(mgcp_msg->codecs)); memcpy(mgcp_msg->ptmap, info->ptmap, sizeof(mgcp_msg->ptmap)); memcpy(&mgcp_msg->param, &info->param, sizeof(mgcp_msg->param)); @@ -173,12 +171,10 @@ .audio_ip = mgcp_ctx->conn_peer_local.addr, .audio_port = mgcp_ctx->conn_peer_local.port, .ptime = mgcp_ctx->conn_peer_local.ptime, - .codecs_len = mgcp_ctx->conn_peer_local.codecs_len, .ptmap_len = mgcp_ctx->conn_peer_local.ptmap_len, .param_present = mgcp_ctx->conn_peer_local.param_present }; osmo_strlcpy(mgcp_msg.endpoint, mgcp_ctx->conn_peer_remote.endpoint, MGCP_ENDPOINT_MAXLEN); - memcpy(mgcp_msg.codecs, mgcp_ctx->conn_peer_local.codecs, sizeof(mgcp_msg.codecs)); memcpy(mgcp_msg.ptmap, mgcp_ctx->conn_peer_local.ptmap, sizeof(mgcp_msg.ptmap)); memcpy(&mgcp_msg.param, &mgcp_ctx->conn_peer_local.param, sizeof(mgcp_ctx->conn_peer_local.param)); @@ -628,6 +624,72 @@ .log_subsys = DLMGCP, }; +/* Provide backwards compat for deprecated conn_peer->codecs[]: when the caller passes in an mgcp_conn_peer instance + * that has codecs[] set, apply it to ptmap[] instead. */ +static void mgcp_conn_peer_compat(struct mgcp_conn_peer *conn_peer) +{ + struct ptmap ptmap[MGCP_MAX_CODECS]; + unsigned int ptmap_len; + + if (!conn_peer->codecs_len) + return; + + /* Before dropping codecs[], codecs[] would indicate the order in which the codecs should appear in SDP. ptmap[] + * would indicate payload type numbers when not using a default payload type number (may omit entries). + * Now, ptmap[] just indicates both at the same time; codecs[] should be empty, and ptmap[] lists all codecs. + * So if any codecs[] are present, recreate ptmap[] in the order of codecs[]. */ + + ptmap_len = 0; + for (int i = 0; i < conn_peer->codecs_len; i++) { + enum mgcp_codecs codec = conn_peer->codecs[i]; + struct ptmap *found = NULL; + + /* Look up whether a specific pt was indicated for this codec */ + for (int p = 0; p < conn_peer->ptmap_len; p++) { + if (conn_peer->ptmap[p].codec != codec) + continue; + found = &conn_peer->ptmap[p]; + break; + } + if (found) { + ptmap[ptmap_len] = *found; + } else { + ptmap[ptmap_len] = (struct ptmap){ + .codec = codec, + /* some enum mgcp_codecs correspond to their standard PT nr, so for compat: */ + .pt = codec, + }; + } + ptmap_len++; + } + + /* Are there any entries in the old ptmap that were omitted by codecs[]? */ + for (int p = 0; p < conn_peer->ptmap_len; p++) { + bool exists = false; + for (int i = 0; i < ptmap_len; i++) { + if (ptmap_cmp(&ptmap[i], &conn_peer->ptmap[p])) + continue; + exists = true; + break; + } + + if (exists) + continue; + + if (ptmap_len >= ARRAY_SIZE(ptmap)) + break; + + /* Not present yet, add it to the end */ + ptmap[ptmap_len] = conn_peer->ptmap[p]; + ptmap_len++; + } + + /* Use the new ptmap[], and clear out legacy codecs[]. */ + memcpy(conn_peer->ptmap, ptmap, sizeof(conn_peer->ptmap)); + conn_peer->ptmap_len = ptmap_len; + conn_peer->codecs_len = 0; +} + /*! allocate FSM, and create a new connection on the MGW. * \param[in] mgcp MGCP client descriptor. * \param[in] parent_fi Parent FSM instance. @@ -642,6 +704,7 @@ struct osmo_fsm_inst *fi; struct in6_addr ip_test; + mgcp_conn_peer_compat(conn_peer); OSMO_ASSERT(parent_fi); OSMO_ASSERT(mgcp); @@ -681,6 +744,8 @@ struct mgcp_ctx *mgcp_ctx = fi->priv; struct in6_addr ip_test; + mgcp_conn_peer_compat(conn_peer); + OSMO_ASSERT(mgcp_ctx); OSMO_ASSERT(conn_peer); diff --git a/tests/mgcp_client/mgcp_client_test.c b/tests/mgcp_client/mgcp_client_test.c index a39f19b..3883c3a 100644 --- a/tests/mgcp_client/mgcp_client_test.c +++ b/tests/mgcp_client/mgcp_client_test.c @@ -107,9 +107,6 @@ printf(" audio_port = %u\n", response->audio_port); printf(" audio_ip = %s\n", response->audio_ip); printf(" ptime = %u\n", response->ptime); - printf(" codecs_len = %u\n", response->codecs_len); - for(i=0;i<response->codecs_len;i++) - printf(" codecs[%u] = %u\n", i, response->codecs[i]); printf(" ptmap_len = %u\n", response->ptmap_len); for(i=0;i<response->ptmap_len;i++) { printf(" ptmap[%u].codec = %u\n", i, response->ptmap[i].codec); @@ -149,12 +146,11 @@ .conn_id = "11", .conn_mode = MGCP_CONN_RECV_SEND, .ptime = 20, - .codecs[0] = CODEC_GSM_8000_1, - .codecs[1] = CODEC_AMR_8000_1, - .codecs[2] = CODEC_GSMEFR_8000_1, - .codecs_len = 1, - .ptmap[0].codec = CODEC_GSMEFR_8000_1, - .ptmap[0].pt = 96, + .ptmap = { + { .codec = CODEC_GSM_8000_1, .pt = CODEC_GSM_8000_1 }, + { .codec = CODEC_AMR_8000_1, .pt = CODEC_AMR_8000_1 }, + { .codec = CODEC_GSMEFR_8000_1, .pt = 96 }, + }, .ptmap_len = 1, .x_osmo_ign = MGCP_X_OSMO_IGN_CALLID, .x_osmo_osmux_cid = -1, /* wildcard */ @@ -179,9 +175,9 @@ mgcp_msg.presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE); - mgcp_msg.codecs_len = 2; + mgcp_msg.ptmap_len = 2; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated CRCX message (three codecs, one with custom pt):\n"); @@ -189,9 +185,9 @@ mgcp_msg.presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE); - mgcp_msg.codecs_len = 3; + mgcp_msg.ptmap_len = 3; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated MDCX message:\n"); @@ -209,9 +205,9 @@ (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE | MGCP_MSG_PRESENCE_AUDIO_IP | MGCP_MSG_PRESENCE_AUDIO_PORT); - mgcp_msg.codecs_len = 2; + mgcp_msg.ptmap_len = 2; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated MDCX message (three codecs, one with custom pt):\n"); @@ -220,9 +216,9 @@ (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE | MGCP_MSG_PRESENCE_AUDIO_IP | MGCP_MSG_PRESENCE_AUDIO_PORT); - mgcp_msg.codecs_len = 3; + mgcp_msg.ptmap_len = 3; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated DLCX message:\n"); @@ -330,8 +326,10 @@ .presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE), .ptime = 20, - .codecs[0] = CODEC_AMR_8000_1, - .codecs_len = 1 + .ptmap = { + { .codec = CODEC_AMR_8000_1, .pt = CODEC_AMR_8000_1 }, + }, + .ptmap_len = 1 }; printf("\n%s():\n", __func__); -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34899?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: I798e02c6663376d3d52f4a74fc4b32411ce95bed Gerrit-Change-Number: 34899 Gerrit-PatchSet: 12 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: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[L] Change in osmo-mgw[master]: client: collapse codecs[] and ptmap[]; allow codec variants
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: fixeria, laforge, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34899?usp=email ) Change subject: client: collapse codecs[] and ptmap[]; allow codec variants ...................................................................... Patch Set 12: Code-Review+2 (1 comment) Patchset: PS12: re-apply votes after adding a TODO-RELEASE entry -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34899?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: I798e02c6663376d3d52f4a74fc4b32411ce95bed Gerrit-Change-Number: 34899 Gerrit-PatchSet: 12 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: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Fri, 05 Jan 2024 01:46:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-mgw[master]: client: allow MGCP_MAX_CODECS entries
by neels 05 Jan '24

05 Jan '24

Attention is currently required from: neels. Hello Jenkins Builder, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/35420?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder The change is no longer submittable: Verified is unsatisfied now. Change subject: client: allow MGCP_MAX_CODECS entries ...................................................................... client: allow MGCP_MAX_CODECS entries So far we allow only MGCP_MAX_CODECS-1 entries, because the parsing exit condition hits only after the array size check. Instead, check the array size a bit later, just before actually adding a valid entry. This is verified to work as expected in upcoming patch I842ce65a9a70f313570857b7df53727cc572b9e6 that adds a new mgcp_client_test.c section for this. Change-Id: I9a28da85e437f118026ea71a5a708e5758fff623 --- M src/libosmo-mgcp-client/mgcp_client.c 1 file changed, 21 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/20/35420/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35420?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: I9a28da85e437f118026ea71a5a708e5758fff623 Gerrit-Change-Number: 35420 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0