Attention is currently required from: dexter, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email )
Change subject: mgcp_client_fsm: fix inconsistent API (param_present, param).
......................................................................
Patch Set 8:
(1 comment)
File include/osmocom/mgcp/mgcp_common.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/3befaf34_62063e21
PS8, Line 73: } amr;
> let's add a char* fmtp entry to struct ptmap instead. […]
(that fmtp_get_int() idea would also need another argument: a default value in case the fmtp in question is omitted)
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?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: I50d737f3f3d45e4004c64101700a471fe75b3436
Gerrit-Change-Number: 34351
Gerrit-PatchSet: 8
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 25 Oct 2023 21:45:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: dexter, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email )
Change subject: mgcp_client_fsm: fix inconsistent API (param_present, param).
......................................................................
Patch Set 8:
(1 comment)
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/2d415c7c_33c76717
PS8, Line 1374: OSMO_ASSERT(!(mgcp_msg->param_present && mgcp_msg->codecs_param_present));
> when we add a fmtp string to ptmap, then we don't need a codecs_param list nor a codecs_param_presen […]
hmm, can we assume to control all existing osmo_mgcp_cient users? it would be very tempting to just drop the stupid 'param' completely, because its compatibility code would have to compose fmtp strings again... just thinking, probably not going to happen, right
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?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: I50d737f3f3d45e4004c64101700a471fe75b3436
Gerrit-Change-Number: 34351
Gerrit-PatchSet: 8
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 25 Oct 2023 21:43:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: dexter, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34351?usp=email )
Change subject: mgcp_client_fsm: fix inconsistent API (param_present, param).
......................................................................
Patch Set 8: Code-Review-1
(7 comments)
Patchset:
PS8:
this can be much simpler, especially after https://gerrit.osmocom.org/c/osmo-mgw/+/34899 ...
File include/osmocom/mgcp/mgcp_common.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/169b3632_9f718fee
PS8, Line 73: } amr;
let's add a char* fmtp entry to struct ptmap instead.
To go with it, let's add individual parsing functions like
bool oa = (fmtp_get_int(ptmap->fmtp, "octet-align") == 1)
The parsing should only happen when we actually need details on an AMR entry, not on every message parsing and composition.
why:
so what i don't like about this is that it needs to model each and every fmtp that exists in anyone's MGCP in a C struct. Much rather, I'd suggest that we simply keep an actual string of the fmtp, and evaluate that string whenever we would need the conveyed information. Like that we can transport and respond with fmtp that osmo-mgw does not even need to care about. With the C struct implementation, we silently drop all unknown fmtp in a conversation, or we need to error a lot.
File include/osmocom/mgcp_client/mgcp_client_fsm.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/ba48c374_58bbd033
PS2, Line 70: struct mgcp_codec_param2 codecs_param[MGCP_MAX_CODECS];
> Ok, better sort that kind of problems *before* this patch.
Instead please add a fmtp entry to the struct ptmap, so that all of pt, codec and fmtp are kept in the same place. (further below i explain why i think it should be a string)
why:
i don't like this at all at all, it creates a separate array that needs to be kept in sync with (in this patch) the 'codecs' list as well as the 'ptmap' list. While doing this review, i submitted a patch to combine those two into just 'ptmap', because it is complete madness to keep N separate arrays of individual struct members, when you can just keep one array of structs. It would be cool to continue in that way.
File src/libosmo-mgcp-client/mgcp_client.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/3e14e32c_d1270c9c
PS8, Line 1374: OSMO_ASSERT(!(mgcp_msg->param_present && mgcp_msg->codecs_param_present));
when we add a fmtp string to ptmap, then we don't need a codecs_param list nor a codecs_param_present flag. Instead I would see if in an incoming mgcp_msg, if param_present == true, apply that param only to those ptmap entries where there is no fmtp yet.
It would be cooler if we could just drop param, but we need to be backwards compatible, right?
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/0aefc4f3_c31feb32
PS8, Line 1391: for (i = 0; i < mgcp_msg->codecs_len; i++) {
if you accept my suggestion in https://gerrit.osmocom.org/c/osmo-mgw/+/34899 then here you'd iterate mgcp_msg->ptmap instead.
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/d38e13de_a81a82c0
PS8, Line 1394: continue;
so this is dropping all fmtp except for AMR codecs, not ideal. Instead we could just simply store the string as it is.
https://gerrit.osmocom.org/c/osmo-mgw/+/34351/comment/4a071b36_1af1b90e
PS8, Line 1399: MSGB_PRINTF_OR_RET("a=fmtp:%u octet-align=0\r\n", pt);
i don't like this: this is implementing codec-specific fmtp composition in osmo-mgw. Instead we should use the exact string that the user provided.
If we take on fmtp composition, we are opening a can of worms.
For example, the octet-align parameter is specified with a default value. It is usually only present when it should differ from the default, and omitted if it is the default. I'm not saying which one is better -- the point is that we don't want to have to make these decisions in osmo-mgw, at all.
If we keep the fmtp string 1:1, we can just lean back and use whatever clients throw at us, just carry the fmtp along as they are. Only when we need a fmtp detail like for AMR OA vs BE conversion, only then do we need to validate that the fmtp parameters make any sense. Only then will we have to throw an error if the fmtp is garbage. For those cases we don't care whether a default value is present or omitted, we just need to get the bit of message handling relevant information from it.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34351?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: I50d737f3f3d45e4004c64101700a471fe75b3436
Gerrit-Change-Number: 34351
Gerrit-PatchSet: 8
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 25 Oct 2023 21:39:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels.
pespin 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/9ebb4e86_7aea0f98
PS1, Line 321: /* Do not allow excessive payload types */
why is this r->ptmap now? I see you are acessing r->codecs[count] below...
--
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: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 25 Oct 2023 21:14:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: dexter.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34350?usp=email )
Change subject: mgcp_client_fsm: explain member param in struct mgcp_conn_peer better
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
ok, though it is a bit weird to improve the documentation for a struct member that we need to replace (in order to allow varying fmtp per ptmap entry)
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34350?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: Iea4dc1e72fccaa464ce503fae88b5d8a867b1d19
Gerrit-Change-Number: 34350
Gerrit-PatchSet: 4
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 25 Oct 2023 20:58:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Jenkins Builder,
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 (#3).
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/3
--
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: 3
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
Hello Jenkins Builder,
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 (#2).
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.
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, 108 insertions(+), 63 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/99/34899/2
--
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: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
Attention is currently required from: dexter, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34349?usp=email )
Change subject: mgcp_client_fsm: allow the same codec multiple times in ptmap
......................................................................
Patch Set 5: Code-Review-2
(1 comment)
Patchset:
PS5:
i see the actual problem this patch is working around ... let's do this instead:
https://gerrit.osmocom.org/c/osmo-mgw/+/34899
ttcn3 test suite says it's good.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34349?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: Ie13ce59d3165936a16e16588b4d58f0ce7e0ae67
Gerrit-Change-Number: 34349
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 25 Oct 2023 20:27:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment