October 2023 - gerrit-log - lists.osmocom.org

[M] Change in osmo-mgw[master]: client: collapse codecs[] and ptmap[]; allow codec variants
by neels 26 Oct '23

26 Oct '23

Attention is currently required from: 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 4: (1 comment) File src/libosmo-mgcp-client/mgcp_client.c: https://gerrit.osmocom.org/c/osmo-mgw/+/34899/comment/3a105493_58c6be0c PS4, Line 398: if (r->ptmap_len >= ARRAY_SIZE(r->ptmap)) { this check needs to be dropped, because we're just adding info to an existing item (normally) -- 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: 4 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 23:58:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in osmo-mgw[master]: add fmtp string to ptmap: allow all possible fmtp
by neels 26 Oct '23

26 Oct '23

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 3: (3 comments) File include/osmocom/mgcp/mgcp_network.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/26f6967e_d57f2eb6 PS3, Line 90: char fmtp[256]; > here is that char fmtp[256], it matches the storage choice of audio_name[] above. context: this is used in public API, also as out-parameter. Dynamic allocation would be tricky, so leave it static. File include/osmocom/mgcp_client/mgcp_client.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/ae818ca6_8024f899 PS3, Line 84: const char *fmtp; > I'm a bit wobbly on the storage choice. Elsewhere I put a char fmtp[256]. […] context: structs with ptmap members are: - struct mgcp_conn_peer - struct mgcp_response If a fmtp is only passed in as function arg, it is fine to be a static pointer that goes bust after the function returns. For persistent storage, a pointer going bust would be bad. So are any of these structs persistent? mgcp_conn_peer is stored persistently in at least mgcp_client_endpoint_fsm.c. It copies a value passed in from a caller outside of this library. So indeed a static array would be safer / simpler here. File src/libosmo-mgcp/mgcp_sdp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/8333a45b_d2dc732c PS3, Line 59: const char *fmtp; > here's another dynamic string... context: this struct is used only internally in this .c file. The storage is never persistent, always limited in a function scope. There already is a bunch of talloc allocation with a temporary ctx, so this is fine to remain a pointer (and not impose a length limit). -- 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: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 23:26:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[L] Change in osmo-mgw[master]: add fmtp string to ptmap: allow all possible fmtp
by neels 26 Oct '23

26 Oct '23

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 3: (3 comments) File include/osmocom/mgcp/mgcp_network.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/7a20c06c_c53adb21 PS3, Line 90: char fmtp[256]; here is that char fmtp[256], it matches the storage choice of audio_name[] above. File include/osmocom/mgcp_client/mgcp_client.h: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/e714e348_5c276466 PS3, Line 84: const char *fmtp; I'm a bit wobbly on the storage choice. Elsewhere I put a char fmtp[256]. So maybe also an array here instead of a pointer? It would make those structs using this completely self-contained. Does 256 seem like a sane limit? dynamic string would have no limit... opinions welcome. File src/libosmo-mgcp/mgcp_sdp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/b572064d_fb311e6a PS3, Line 59: const char *fmtp; here's another dynamic string... -- 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: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 23:06:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in osmo-mgw[master]: mgcp_parse_audio_port_pt(): fix buffer overflow
by neels 26 Oct '23

26 Oct '23

Attention is currently required from: pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34898?usp=email ) Change subject: mgcp_parse_audio_port_pt(): fix buffer overflow ...................................................................... Patch Set 2: (1 comment) File src/libosmo-mgcp-client/mgcp_client.c: https://gerrit.osmocom.org/c/osmo-mgw/+/34898/comment/08e1aff1_0e1870e6 PS1, Line 321: /* Do not allow excessive payload types */ > oh, artifact from a future patch! good catch, thanks! Done -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34898?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: I18c78d15eb1593f404b4741248225b68878b463f Gerrit-Change-Number: 34898 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 23:00:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[L] Change in osmo-mgw[master]: add fmtp string to ptmap: allow all possible fmtp
by neels 26 Oct '23

26 Oct '23

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 3: (2 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/34900/comment/aa1a455b_57348244 PS2, Line 20: if (osmo_mgcp_fmtp_get_int("octet-aligned", 0) == 1) ... > I just noticed there is confusion in the code about the name of this attribute: […] Done Patchset: PS1: > ttcn3 tests still fail, @pmaier@sysmocom.de would be great if you could take a look.. […] Done -- 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: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 22:56:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[L] Change in osmo-mgw[master]: add fmtp string to ptmap: allow all possible fmtp
by neels 26 Oct '23

26 Oct '23

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 3: (1 comment) This change is ready for review. Patchset: PS3: tests pass now -- 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: 3 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 22:56:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in osmo-mgw[master]: mgcp_parse_audio_port_pt(): fix buffer overflow
by neels 26 Oct '23

26 Oct '23

Attention is currently required from: pespin. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/34898?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder Change subject: mgcp_parse_audio_port_pt(): fix buffer overflow ...................................................................... mgcp_parse_audio_port_pt(): fix buffer overflow Change-Id: I18c78d15eb1593f404b4741248225b68878b463f --- M src/libosmo-mgcp-client/mgcp_client.c 1 file changed, 10 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/98/34898/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34898?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: I18c78d15eb1593f404b4741248225b68878b463f Gerrit-Change-Number: 34898 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-mgw[master]: client: collapse codecs[] and ptmap[]; allow codec variants
by neels 26 Oct '23

26 Oct '23

Attention is currently required from: neels, pespin. Hello Jenkins Builder, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/34899?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+1 by pespin, Verified+1 by Jenkins Builder 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, in both struct mgcp_response and struct mgcp_msg: - codecs[], codecs_len stores the 'm=audio' list - ptmap[], ptmap_len stores the 'a=rtpmap' list These are semantically identical, and the separation means we cannot have the same codec twice, like AMR with different fmtp variations. It also 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. Related: OS#6171 Change-Id: I798e02c6663376d3d52f4a74fc4b32411ce95bed --- M include/osmocom/mgcp_client/mgcp_client.h M include/osmocom/mgcp_client/mgcp_client_fsm.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 5 files changed, 109 insertions(+), 63 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/99/34899/4 -- 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: 4 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-mgw[master]: invent fmtp 'gsm-hr-format', for RFC5993 vs 3GPP TS 101.318
by neels 26 Oct '23

26 Oct '23

Attention is currently required from: laforge, neels. neels has uploaded a new patch set (#11) to the change originally created by dexter. ( https://gerrit.osmocom.org/c/osmo-mgw/+/31214?usp=email ) The following approvals got outdated and were removed: Code-Review-1 by neels, Verified+1 by Jenkins Builder Change subject: invent fmtp 'gsm-hr-format', for RFC5993 vs 3GPP TS 101.318 ...................................................................... invent fmtp 'gsm-hr-format', for RFC5993 vs 3GPP TS 101.318 There are two different RTP HR GSM formats defined: - 3GPP TS 101.318 - RFC5993 They differ by one zero padding byte at the start of the RTP payload. Conversion code already exists prior to this patch. To allow configuring the conversion from MGCP clients, invent a new osmocom-specific fmtp parameter for GSM-HR: gsm-hr-format=3gpp-ts-101.318 gsm-hr-format=rfc5993 If an MGCP client indicates this parameter in the GSM-HR's fmtp and an incoming RTP GSM HR packet mismatches that format, convert to the indicated format. An SDP example: m=audio 1234 RTP/AVP 111 a=rtpmap:111 GSM-HR-08/8000/1 a=fmtp:111 gsm-hr-format=3gpp-ts-101.318 Patch-by: pmaier nhofmeyr Change-Id: Idde8da27fd335dc03b8fbd9e0fedc1491b77e9e4 Related: OS#5688 --- M include/osmocom/mgcp/fmtp.h M include/osmocom/mgcp/mgcp_common.h M src/libosmo-mgcp/fmtp.c M src/libosmo-mgcp/mgcp_network.c 4 files changed, 131 insertions(+), 10 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/14/31214/11 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/31214?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: Idde8da27fd335dc03b8fbd9e0fedc1491b77e9e4 Gerrit-Change-Number: 31214 Gerrit-PatchSet: 11 Gerrit-Owner: dexter <pmaier(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: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-mgw[master]: mgcp_parse_audio_port_pt(): fix buffer overflow
by neels 26 Oct '23

26 Oct '23

Attention is currently required from: pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34898?usp=email ) Change subject: mgcp_parse_audio_port_pt(): fix buffer overflow ...................................................................... Patch Set 1: (1 comment) File src/libosmo-mgcp-client/mgcp_client.c: https://gerrit.osmocom.org/c/osmo-mgw/+/34898/comment/e7c40dda_92252c49 PS1, Line 321: /* Do not allow excessive payload types */ > why is this r->ptmap now? I see you are acessing r->codecs[count] below... oh, artifact from a future patch! good catch, thanks! -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34898?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: I18c78d15eb1593f404b4741248225b68878b463f Gerrit-Change-Number: 34898 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 26 Oct 2023 21:02:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0