Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/10172 )
Change subject: mgcp_network: translate payload type numbers in RTP packets ...................................................................... Patch Set 1: Code-Review-1 (12 comments) mainly good, let's resolve the few details https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c File src/libosmo-mgcp/mgcp_codec.c: https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@345 PS1, Line 345: /* Helper function to compare two codecs, all parameters must match up, except (I also used to call everything a "helper function" until I realized *everything* except main() is a helper function, and the only meaningful part of the comment is a description of the actual API) https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@370 PS1, Line 370: * \param[in] conn_src related destination rtp-connection. "conn_dst" I mean, we don't have doxygen here anyway, but if you're writing doxygen then it should be correct :) https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@373 PS1, Line 373: int mgcp_codec_pt_translate(struct mgcp_conn_rtp *conn_src, the caller passes in a uint8_t payload_type and masks the returned value with 0xff into a uint8_t. Is there a good reason for this being an int param and return val? https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@388 PS1, Line 388: OSMO_ASSERT(codecs_assigned < MGCP_MAX_CODECS) fix ';' and indenting of 'for' below. Shouldn't it be "<=" the max? https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@401 PS1, Line 401: OSMO_ASSERT(codecs_assigned < MGCP_MAX_CODECS) same '<=' (?) and ';' and indent https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c File src/libosmo-mgcp/mgcp_network.c: https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@489 PS1, Line 489: OSMO_ASSERT(len >= sizeof(struct rtp_hdr)); where does the len come from? If it's a len influenced by the size of an incoming packet, we must not assert = must not kill the program if it's too small. https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@497 PS1, Line 497: rtp_hdr->payload_type = (uint8_t) (pt_out & 0xFF); (when casting to uint8_t, the & 0xff is implicit and your & 0xFF has no effect) https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@718 PS1, Line 718: * should not occurr if transcoding is consequently avoided. Until "occur" https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@719 PS1, Line 719: * we do not have transcoding support in osmo-mgw we can not resolve "Until we have" https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c File tests/mgcp/mgcp_test.c: https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c@1619 PS1, Line 1619: static void test_mgcp_codec_pt_translate_pars(struct mgcp_rtp_codec *c) (looks like a static const struct implemented as a function?) https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c@1662 PS1, Line 1662: conn_dst.end.codecs_assigned = 3; (IMHO a bit weird way of initializing three structs, but ok) https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c@1665 PS1, Line 1665: * packet out to the destination. All we know the context for both "is"? -- To view, visit https://gerrit.osmocom.org/10172 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a874e59fa07bcc2a67c376cafa197360036f539 Gerrit-Change-Number: 10172 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Comment-Date: Thu, 26 Jul 2018 21:16:42 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
