<p style="white-space: pre-wrap; word-wrap: break-word;">(ensure all comments are sent)</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898">View Change</a></p><p>19 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c">File src/libosmo-mgcp/mgcp_endp.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/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@81">Patch Set #4, Line 81:</a> <code style="font-family:monospace,monospace">               { 0, 0, 4, 0, 2, 4, 6, 0, 1, 2, 3, 4, 5, 6, 7 };</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(are these the same numbers as below const arrays? if yes please don't dup but use a common global c […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@373">Patch Set #4, Line 373:</a> <code style="font-family:monospace,monospace">   (e.g. ds/e1-0/s-30/su16-4), returns 0xff on error. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(comment convention: start all lines with '*', same below)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@374">Patch Set #4, Line 374:</a> <code style="font-family:monospace,monospace">static uint8_t e1_ts_nr_from_epname(char *epname)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const char *epname, same below</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@410">Patch Set #4, Line 410:</a> <code style="font-family:monospace,monospace">    static const uint8_t rates[] = { 64, 32, 16, 16, 8 };</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(global? like commented below)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@468">Patch Set #4, Line 468:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The above e1_xxx_from_epname() each tokenize and parse the same epname, one after the other. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I thought about doing that while I was doing the implementation, but I prefer to have a single function for each parameter.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@485">Patch Set #4, Line 485:</a> <code style="font-family:monospace,monospace">         { 0, 0, 4, 0, 2, 4, 6, 0, 1, 2, 3, 4, 5, 6, 7 };</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">maybe these two const arrays should be defined at the top of the .c file? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@487">Patch Set #4, Line 487:</a> <code style="font-family:monospace,monospace">      OSMO_ASSERT(sizeof(rates) == sizeof(offsets));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this should use an osmo_static_assert() (compile time)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@506">Patch Set #4, Line 506:</a> <code style="font-family:monospace,monospace">      * rate in the form: i:r where i is the subsolt number and r the</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(subsolt x2)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@534">Patch Set #4, Line 534:</a> <code style="font-family:monospace,monospace">     { 0, 3, 4, 7, 8, 9, 10, -1, -1, -1, -1, -1, -1, -1, -1 },</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(maybe decide on indent or no indent)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@563">Patch Set #4, Line 563:</a> <code style="font-family:monospace,monospace">   if (ts_nr == 0xff || ss_nr == 0xff) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(here is a place that would look nicer to me if it was just a single conversion function... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@571">Patch Set #4, Line 571:</a> <code style="font-family:monospace,monospace">   for (i = 0; i < 15; i++) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(could use sizeof(interlock_tab[0]) instead of 15?)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@572">Patch Set #4, Line 572:</a> <code style="font-family:monospace,monospace">         /* Detect table end */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(more like row end?)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@591">Patch Set #4, Line 591:</a> <code style="font-family:monospace,monospace">                * (This is an exceptional situation, that should not occurr</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(occur)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@592">Patch Set #4, Line 592:</a> <code style="font-family:monospace,monospace">               * in a properly configured environment!) */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">IIUC then this function is called by e.g. a CRCX message coming from outside of osmo-mgw. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If this happens its not a problem at the mgw side. osmo-mgw will reject the request with an error code. Its the requestor that has a problematic configuration or malfunction then.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@611">Patch Set #4, Line 611:</a> <code style="font-family:monospace,monospace">               /* There are no obsticels that may render a virtual trunk</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(obstacles)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@614">Patch Set #4, Line 614:</a> <code style="font-family:monospace,monospace">             return true;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">but a virtual endpoint could also already be in use?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">In "in use" means something different here. "Available" just means that the client can do something with the endpoint (CRCX, DLCX, MDCX etc.). Its more like an "in service" or "out of service" thing.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c">File src/libosmo-mgcp/mgcp_ratectr.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/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c@66">Patch Set #4, Line 66:</a> <code style="font-family:monospace,monospace">      [MGCP_CRCX_FAIL_AVAIL] = { "crcx:availability", "endpoint unavailable." },</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">i'd prefer 'crcx:unavailable' (otherwise sounds like it counts successful CRCX) […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c@94">Patch Set #4, Line 94:</a> <code style="font-family:monospace,monospace"> [MGCP_MDCX_FAIL_AVAIL] = { "mdcx:availability", "endpoint unavailable." },</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">MDCX only makes sense for conns that are already active. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It does not matter if MDCX, CRCX etc would succeed or not, this is checked much later. The question is, does the client try to something with an endpoint that is currently not in service. (e.g. The linecard for this endpoint is not installed in the rack.). If there is an active call on an endpoint and if it is not allowed to create another connection, than the endpoint is "available" and the operation will fail for a different reason.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it fails earlier and the _NO_CONN counter does not tick. I added the availablity as a separate counter because especially with the E1 the problem could be difficult to diagnose. Its an unlikely situation that the bsc will mess up the endpoints, but if it ever happens we need to be able spot this immediately.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c@116">Patch Set #4, Line 116:</a> <code style="font-family:monospace,monospace">      [MGCP_DLCX_FAIL_AVAIL] = { "dlcx:availability", "endpoint unavailable." },</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same as for MDCX</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898">change 18898</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/c/osmo-mgw/+/18898"/><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-Change-Id: I18e90b10648a7e504371179ad144645fc82e1c27 </div>
<div style="display:none"> Gerrit-Change-Number: 18898 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </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: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 29 Jun 2020 15:31:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>