<p style="white-space: pre-wrap; word-wrap: break-word;">(ensuring all messages are sent)</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19121">View Change</a></p><p>10 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/+/19121/2/include/osmocom/mgcp/mgcp_trunk.h">File include/osmocom/mgcp/mgcp_trunk.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/+/19121/2/include/osmocom/mgcp/mgcp_trunk.h@51">Patch Set #2, Line 51:</a> <code style="font-family:monospace,monospace"> } virtual;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">better to use a different name, because 'virtual' is a C++ keyword... virt or virtual_trunk or ... […]</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/+/19121/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/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@51">Patch Set #2, Line 51:</a> <code style="font-family:monospace,monospace"> trunk->trunk_nr = nr;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(unrelated whitespace change)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is intentional. I want to have the trunk specific and the generic settings grouped.</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/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@87">Patch Set #2, Line 87:</a> <code style="font-family:monospace,monospace"> offset_endpoints = 1;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(naming: first_endp_id or first_endp_nr? "offset" to me indicates that we are skipping some items in […]</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/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@99">Patch Set #2, Line 99:</a> <code style="font-family:monospace,monospace"> * however miss-initalation of the struct or memory corruption</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">'initialization'</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/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@103">Patch Set #2, Line 103:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Note that the vty allows vty_number_endpoints to be 0, in which case we probably should not invoke t […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should make sure that the VTY forces to create at least one endpoints. I have fixed it there now.</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/+/19121/2/src/libosmo-mgcp/mgcp_trunk.c@105">Patch Set #2, Line 105:</a> <code style="font-family:monospace,monospace"> trunk->endpoints = _talloc_zero_array(trunk->cfg,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(separate issue: seems sketchy to use an API function that starts with an underscore and that isn't […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have fixed that in a different patch.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19121/3/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/+/19121/3/src/libosmo-mgcp/mgcp_trunk.c@45">Patch Set #3, Line 45:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">trunk->virtual.vty_number_endpoints = 32;<br> trunk->e1.vty_timeslot_first = 1;<br> trunk->e1.vty_timeslot_num = 2;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would not make this vty configurable. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just thought that a bit of flexibility would not hurt. So we make it static: first timeslot is 1 and last one is 32. Maybe it would be a compromise to put a default config that opens all timeslots. Then we would not need to syncronize the configs, but should it be necessary we have the option to do so.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19121/2/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/+/19121/2/src/libosmo-mgcp/mgcp_vty.c@713">Patch Set #2, Line 713:</a> <code style="font-family:monospace,monospace"> "number endpoints <0-65534>",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">...this is where vty_number_endpoints == 0 is allowed by the vty. […]</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/+/19121/2/src/libosmo-mgcp/mgcp_vty.c@936">Patch Set #2, Line 936:</a> <code style="font-family:monospace,monospace"> vty_out(vty, " timeslot first %u%s",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Preferably add new vty commands in a separate patch, because the commit log says "refactor"</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You are probably right, this patch is more about adding features. The refactoring is just a minimal side effect.</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/+/19121/2/src/libosmo-mgcp/mgcp_vty.c@1179">Patch Set #2, Line 1179:</a> <code style="font-family:monospace,monospace"> "Timeslot options\n" "Number of E1 timeslots to use\n" "E1 timeslot count\n")</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">explain whether the used E1 timeslots end up being [first .. first+num] or [first .. […]</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/+/19121">change 19121</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/+/19121"/><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: I73b31e3c236a61ea0a6f76ef5ff98ce589f52c77 </div>
<div style="display:none"> Gerrit-Change-Number: 19121 </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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 07 Jul 2020 14:10:47 +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"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>