<p>Patch set 6:<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/c/osmo-mgw/+/19103">View Change</a></p><p>2 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/+/19103/6/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/+/19103/6/src/libosmo-mgcp/mgcp_vty.c@858">Patch Set #6, Line 858:</a> <code style="font-family:monospace,monospace">      "trunk e1 <1-64>", "Configure Trunk\n" "E1 trunk\n" "Trunk Nr\n")</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">do we need compatibility for old 'trunk <1-64>' command?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think so, because the existing code in osmo-mgw for E1 trunks did not work anyway.  So there cannot be any people with [reasonable, working] config files that end up in trouble here.</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/+/19103/6/src/libosmo-mgcp/mgcp_vty.c@897">Patch Set #6, Line 897:</a> <code style="font-family:monospace,monospace">         vty_out(vty, " trunk-e1 %d%s", trunk->trunk_nr, VTY_NEWLINE);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">above you wrote "trunk e1" so I doubt this can be parsed</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we may also want some kind of dispatch here.  After all, we iterate over the global list of trunks (g_cfg->trunks) and not just the list of E1 trunks.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So either </p><p style="white-space: pre-wrap; word-wrap: break-word;">a )the list always only contains E1 trunks (which it doesn't, as the virtual trunk is part of the list).  If this were true, we could keep this patch as-is but rename the list to g_cfg->e1_trunks.</p><p style="white-space: pre-wrap; word-wrap: break-word;">b) the list contains all kinds of trunks.  In this case,</p><ul><li>if we have a E1 trunk specific node, and a specific config_write_ for E1, we ust also make sure that we actually only dump trunks of E1 Type here, and skip all others</li></ul><ul><li>alternatively, keep the node name and hence also the config_write function shared (non E1 specific), but simply have a switch statement inside this function dispatching different trunk types.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">This current patch is, IMHO, creating a more confusing situation than we currently have.  We should be careful to avoid this.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I personally think that we should keep a common/shared trunk node and not rename the NODE define / name nor this function.  All that should be renamed is the 'trunk ...' VTY command to make it E1 specific, in order for us to be able to introduce compatible etensions later on.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But to be honest, we could also do that even with the existing 'trunk <1-64>', command:  It could later be extended to 'trunk <1-64 [(foo|bar|buz)]' with an implicit default to E1.  or we handle it like we handle bts models in OsmBSC: With an explicit 'type (foo|bar|baz)' command within the 'trunk' node.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19103">change 19103</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/+/19103"/><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: I22c39ee9a36e4e737992c91677f3e315907a4c7e </div>
<div style="display:none"> Gerrit-Change-Number: 19103 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </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: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 09 Jul 2020 10:21:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>