<p style="white-space: pre-wrap; word-wrap: break-word;">oof, sorry for posting so many comments again. It's mainly details, but a lot of them.<br>Most important to me is the code dup / static-ness of magic-number tables and the logic around blocked (not "available") endpoints.</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18898">View Change</a></p><p>24 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 style="white-space: pre-wrap; word-wrap: break-word;">(are these the same numbers as below const arrays? if yes please don't dup but use a common global const array)</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 style="white-space: pre-wrap; word-wrap: break-word;">(comment convention: start all lines with '*', same below)</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 style="white-space: pre-wrap; word-wrap: break-word;">const char *epname, same below</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 style="white-space: pre-wrap; word-wrap: break-word;">(global? like commented below)</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 style="white-space: pre-wrap; word-wrap: break-word;">The above e1_xxx_from_epname() each tokenize and parse the same epname, one after the other.<br>Wouldn't it make more sense to have a single function that extracts all of these items at the same time?<br>(could also include below _ss_nr_)</p><p style="white-space: pre-wrap; word-wrap: break-word;">If it were me I'd make single point of conversion between a struct and a string:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct e1_ep {<br> uint8_t rate;<br> uint8_t offs;<br> uint8_t ts;<br> uint8_t ss_nr;<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *e1_ep_to_str_c(void *ctx, const struct e1_rp *e1_ep);<br> int e1_ep_from_str(struct e1_ep *e1_ep, const char *epname);</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Whether the remaining code stores the string or the e1_ep struct is then arbitrary.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Just an idea, I think it would make for cleaner code overall.</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 style="white-space: pre-wrap; word-wrap: break-word;">maybe these two const arrays should be defined at the top of the .c file?<br>(then it is clear that they are static and also available for other functions that might use them in the future)</p><p style="white-space: pre-wrap; word-wrap: break-word;">A const does not need to be static, just leave it to the compiler to optimize the values used.<br>static only needed if changes to it should survive across re-invocations of the function.</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 style="white-space: pre-wrap; word-wrap: break-word;">this should use an osmo_static_assert() (compile time)</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 style="white-space: pre-wrap; word-wrap: break-word;">(subsolt x2)</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 style="white-space: pre-wrap; word-wrap: break-word;">(maybe decide on indent or no indent)</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@548">Patch Set #4, Line 548:</a> <code style="font-family:monospace,monospace"> };</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(this array could also be global, further up in the .c file. If not, then there is no need to make it 'static', really)</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 style="white-space: pre-wrap; word-wrap: break-word;">(here is a place that would look nicer to me if it was just a single conversion function...)</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 style="white-space: pre-wrap; word-wrap: break-word;">(could use sizeof(interlock_tab[0]) instead of 15?)</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 style="white-space: pre-wrap; word-wrap: break-word;">(more like row end?)</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 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/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 style="white-space: pre-wrap; word-wrap: break-word;">IIUC then this function is called by e.g. a CRCX message coming from outside of osmo-mgw. Then that is rather an error situation that osmo-mgw needs to protect against, and properly configuring osmo-mgw is not related?</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@607">Patch Set #4, Line 607:</a> <code style="font-family:monospace,monospace">bool mgcp_endp_avail(struct mgcp_endpoint *endp)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">But a virtual endpoint could also already be in use?</p><p style="white-space: pre-wrap; word-wrap: break-word;">IIUC this function may be described "check whether an endpoint is blocked by another endpoint (for E1)"?</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 style="white-space: pre-wrap; word-wrap: break-word;">(obstacles)</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 style="white-space: pre-wrap; word-wrap: break-word;">but a virtual endpoint could also already be in use?</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_protocol.c">File src/libosmo-mgcp/mgcp_protocol.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_protocol.c@750">Patch Set #4, Line 750:</a> <code style="font-family:monospace,monospace"> if (!mgcp_endp_avail(endp)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(...this is the CRCX rx I mentioned elsewhere)</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_protocol.c@1011">Patch Set #4, Line 1011:</a> <code style="font-family:monospace,monospace"> if (!mgcp_endp_avail(endp)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">an MDCX is by definition sent for an endpoint that is already in use.<br>wouldn't this check forbid all MDCX for all E1 endpoints?<br>(same for DLCX)</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 style="white-space: pre-wrap; word-wrap: break-word;">i'd prefer 'crcx:unavailable' (otherwise sounds like it counts successful CRCX)<br>or after previous discussion 'crcx:blocked'?</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 style="white-space: pre-wrap; word-wrap: break-word;">MDCX only makes sense for conns that are already active.<br>So either we should get MGCP_MDCX_FAIL_NO_CONN, or the MDCX should succeed, right?</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 style="white-space: pre-wrap; word-wrap: break-word;">same as for MDCX</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_vty.c">File src/libosmo-mgcp/mgcp_vty.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_vty.c@203">Patch Set #4, Line 203:</a> <code style="font-family:monospace,monospace"> mgcp_endp_avail(endp) ? "available" : "not in service", VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">how about those endpoints currently in use? We print "available" for all endpoints, which may be interpreted as not being used. Maybe also use the term "blocked" here?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe don't print anything at all for non-blocked endpoints, only for those that are blocked add a "Blocked by: <other-epname>" ?</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: 4 </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: Fri, 26 Jun 2020 15:16:13 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>