<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 4:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Some of my questions/concerns were not answered yet.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">(forcing comments to be sent)</p><p style="white-space: pre-wrap; word-wrap: break-word;">can you check again?</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372">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/+/18372/2//COMMIT_MSG">Commit Message:</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/+/18372/2//COMMIT_MSG@10">Patch Set #2, Line 10:</a> <code style="font-family:monospace,monospace">implemented in various placed (mostly mgcp_protocol.c). Also we use</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">places</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/+/18372/2//COMMIT_MSG@14">Patch Set #2, Line 14:</a> <code style="font-family:monospace,monospace">The trunk and endpoint handling in osmo-mgw is still very complex and</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This paragraph is repeated.</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/+/18372/2//COMMIT_MSG@25">Patch Set #2, Line 25:</a> <code style="font-family:monospace,monospace"> - rename struct mgcp_trunk_config to mgcp_trunk and "tcfg" to "trunk"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Having an "mgcp_trunk" structure and a "trunk" one looks confusing to me.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the one is the struct name and the other is the symbol name used in the code.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3//COMMIT_MSG">Commit Message:</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/+/18372/3//COMMIT_MSG@30">Patch Set #3, Line 30:</a> <code style="font-family:monospace,monospace"> - get rid of deprecated trunk parameters (leftorvers from the</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">leftovers</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/include/osmocom/mgcp/mgcp_endp.h">File include/osmocom/mgcp/mgcp_endp.h:</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/+/18372/2/include/osmocom/mgcp/mgcp_endp.h@81">Patch Set #2, Line 81:</a> <code style="font-family:monospace,monospace"> /*! Backpointer to the related trunk */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"Backpointer to the trunk this endpoint belongs to" would probably be more clear for newcomers to un […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/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/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_codec.c@289">Patch Set #3, Line 289:</a> <code style="font-family:monospace,monospace"> /* FIXME: implement meaningful checks to make sure that the given codec</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">AFAIU we are adding a regression here by dropping this code?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am very sure it won't introduce a regression. I have double checked it now. In my opinion the code is even introducing major problems.</p><p style="white-space: pre-wrap; word-wrap: break-word;">First of all the removal of the code does not change the behavior of osmo-mgw, the reason for this is because no_audio_transcoding is initalized to 0 and we usually do not switch it on, otherwise we would experience a lot of problems!</p><p style="white-space: pre-wrap; word-wrap: break-word;">So only if no_audio_transcoding is 1 the MGW would check the codec name against a pre-configured constant (vty) and if the requested codec does not match that constant CRCX and MDCX would fail. Those VTY settings were there from the beginning and I do not know why I tried keep them but in my opinion this is a very wired way to deal with the lack of transcoding. Apart from that the relevant VTY commands are already marked as deprecated.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Given that making the is_codec_compatible() check take effect is switched off when we use osmo-mgw I think there is no regression possible. We might even think to remove no_audio_transcoding as well. I am not done yet but I think there are a couple more strange trunk properties that do not fit in our new model.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/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/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@101">Patch Set #2, Line 101:</a> <code style="font-family:monospace,monospace"> (epname, MGCP_ENDPOINT_PREFIX_VIRTUAL_TRUNK,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what if len(epname) < prefix_len? Is strncmp safe here?</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/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@158">Patch Set #2, Line 158:</a> <code style="font-family:monospace,monospace"> * wildarded endpoint searches that picks the next free endpoint on</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">wildcarded</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/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@183">Patch Set #2, Line 183:</a> <code style="font-family:monospace,monospace"> endp = trunk->endpoints[i];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(We should move to a hashtable with key=str and val=ptr at some point now that we use strings. […]</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/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@188">Patch Set #2, Line 188:</a> <code style="font-family:monospace,monospace"> "(trunk:%i) found endpoint: %s\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">%d</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/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@196">Patch Set #2, Line 196:</a> <code style="font-family:monospace,monospace"> "(trunk:%i) Not able to find specified endpoint: %s\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">%d</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/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@254">Patch Set #2, Line 254:</a> <code style="font-family:monospace,monospace"> if (trunk->trunk_type == MGCP_TRUNK_VIRTUAL) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(At some point we should have function pointers to do type-specific stuff. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/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/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@100">Patch Set #3, Line 100:</a> <code style="font-family:monospace,monospace"> if (strlen(epname) <= prefix_len)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So I was asking about strncmp because I don't know if the 2 strings here are null terminated. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">They are null terminated, but I think there is still a problem. strncmp compares UP TO n char s. If the epname is lets say only 3 chars long and by chance these chars match the first 3 chars of the trunk prefix we would get a match too. But now I check the length, so it should be fine.</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/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@109">Patch Set #3, Line 109:</a> <code style="font-family:monospace,monospace"> return epname; </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">trailing whitespace</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/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@216">Patch Set #3, Line 216:</a> <code style="font-family:monospace,monospace"> LOGP(DLMGCP, LOGL_ERROR, "missing domain name in endpoint name \"%s\", expecting '%s'\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">SO you first use double quote for epname, but single quote for cfg->domain?</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/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@226">Patch Set #3, Line 226:</a> <code style="font-family:monospace,monospace"> LOGP(DLMGCP, LOGL_ERROR, "wrong domain name in endpoint name \"%s\", expecting '%s'\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/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/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@285">Patch Set #2, Line 285:</a> <code style="font-family:monospace,monospace"> /* FIXME: We hardcode to pick cfg->virt_trunkl, but </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">trailing whitespace</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/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@288">Patch Set #2, Line 288:</a> <code style="font-family:monospace,monospace"> struct rate_ctr_group *rate_ctrs = trunk->mgcp_general_ctr_group;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The rate_ctrs used below look general and not related to a trunk, so they should mbe moved to be und […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think I understand the problem now. The counters were allocated per trunk before. I did not change it, but when I look at the counters, it really looks like if the intension was to create some per-mgw health monitoring. I have now splatted that so that the counters are separate and global in the cfg.</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/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@996">Patch Set #2, Line 996:</a> <code style="font-family:monospace,monospace"> struct mgcp_endpoint *endp = p->endp;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Swap these two lines: […]</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/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@1220">Patch Set #2, Line 1220:</a> <code style="font-family:monospace,monospace"> struct mgcp_endpoint *endp = p->endp;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same, swap these two lines.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_sdp.c">File src/libosmo-mgcp/mgcp_sdp.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/+/18372/2/src/libosmo-mgcp/mgcp_sdp.c@386">Patch Set #2, Line 386:</a> <code style="font-family:monospace,monospace"> LOGP(DLMGCP, LOGL_NOTICE, "endpoint:%s, failed to add codec\n", endp->name);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I see a lot of endpoint:%s logging, what about introducing LOGPENDP(endp, CAT, "... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_trunk.c">File src/libosmo-mgcp/mgcp_trunk.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/+/18372/2/src/libosmo-mgcp/mgcp_trunk.c@30">Patch Set #2, Line 30:</a> <code style="font-family:monospace,monospace">static const struct rate_ctr_desc mgcp_general_ctr_desc[] = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">As I said, to me lots of these counters are not per-trunk, but global, so they shouldn't be here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/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/osmo-mgw/+/18372/2/tests/mgcp/mgcp_test.c@74">Patch Set #2, Line 74:</a> <code style="font-family:monospace,monospace">#define AUEP1_RET "500 158663169 FAIL\r\n"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What about these changes in behavior? expected? I don't recall seeing any comment in commit deescrip […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The problem is that there are still leftovers of an E1 trunk implementation in osmo-mgw. This implementation was never finished. This tests relates to an endpoint of the old E1 implementation. So since E1 is not working and did never work we now reject anything that goes to an E1 trunk. This is why the AUEP here now fails, which is correct.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/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/osmo-mgw/+/18372/3/tests/mgcp/mgcp_test.c@1461">Patch Set #3, Line 1461:</a> <code style="font-family:monospace,monospace"> /* Allocate 5@mgw and let osmo-mgw pick a codec from the list */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm still wondering why are you changing the behavior of the test. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have checked it now back. (See also comment in mgcp_codec.c) See also CRCX_MULT_GSM_EXACT, there are a lot of codecs configured. Right at the front is one that has payload type 0 assigned to it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have removed the possibility to "force" a specific codec type when no transcoding is available (which is a wired way to deal with transcoding). So now can no longer set an audio_name, so the code will pick the first codec in the list, which has payload_type 0, so as far as the test is concerned it does what is expected.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372">change 18372</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/+/18372"/><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: Ice8aaf03faa2fd99074f8665eea3a696d30c5eb3 </div>
<div style="display:none"> Gerrit-Change-Number: 18372 </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-Comment-Date: Fri, 29 May 2020 11:25:23 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>