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]; 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 &lt;pmaier(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Comment-Date: Wed, 25 Oct 2023 21:39:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Comment-In-Reply-To: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-MessageType: comment