<p style="white-space: pre-wrap; word-wrap: break-word;">mainly good, let's resolve the few details</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/10172">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c">File src/libosmo-mgcp/mgcp_codec.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@345">Patch Set #1, Line 345:</a> <code style="font-family:monospace,monospace">/* Helper function to compare two codecs, all parameters must match up, except</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(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)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@370">Patch Set #1, Line 370:</a> <code style="font-family:monospace,monospace"> *  \param[in] conn_src related destination rtp-connection.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"conn_dst"</p><p style="white-space: pre-wrap; word-wrap: break-word;">I mean, we don't have doxygen here anyway, but if you're writing doxygen then it should be correct :)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@373">Patch Set #1, Line 373:</a> <code style="font-family:monospace,monospace">int mgcp_codec_pt_translate(struct mgcp_conn_rtp *conn_src,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@388">Patch Set #1, Line 388:</a> <code style="font-family:monospace,monospace">  OSMO_ASSERT(codecs_assigned < MGCP_MAX_CODECS)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">fix ';' and indenting of 'for' below.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't it be "<=" the max?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_codec.c@401">Patch Set #1, Line 401:</a> <code style="font-family:monospace,monospace">      OSMO_ASSERT(codecs_assigned < MGCP_MAX_CODECS)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same '<=' (?) and ';' and indent</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c">File src/libosmo-mgcp/mgcp_network.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@489">Patch Set #1, Line 489:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(len >= sizeof(struct rtp_hdr));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@497">Patch Set #1, Line 497:</a> <code style="font-family:monospace,monospace">  rtp_hdr->payload_type = (uint8_t) (pt_out & 0xFF);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(when casting to uint8_t, the & 0xff is implicit and your & 0xFF has no effect)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@718">Patch Set #1, Line 718:</a> <code style="font-family:monospace,monospace">    * should not occurr if transcoding is consequently avoided. Until</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"occur"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/src/libosmo-mgcp/mgcp_network.c@719">Patch Set #1, Line 719:</a> <code style="font-family:monospace,monospace">         * we do not have transcoding support in osmo-mgw we can not resolve</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"Until we have"</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c">File tests/mgcp/mgcp_test.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c@1619">Patch Set #1, Line 1619:</a> <code style="font-family:monospace,monospace">static void test_mgcp_codec_pt_translate_pars(struct mgcp_rtp_codec *c)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(looks like a static const struct implemented as a function?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c@1662">Patch Set #1, Line 1662:</a> <code style="font-family:monospace,monospace">       conn_dst.end.codecs_assigned = 3;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(IMHO a bit weird way of initializing three structs, but ok)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10172/1/tests/mgcp/mgcp_test.c@1665">Patch Set #1, Line 1665:</a> <code style="font-family:monospace,monospace">      * packet out to the destination. All we know the context for both</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"is"?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/10172">change 10172</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/10172"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I3a874e59fa07bcc2a67c376cafa197360036f539 </div>
<div style="display:none"> Gerrit-Change-Number: 10172 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 26 Jul 2018 21:16:42 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>