<p>Patch set 2:<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/+/19121">View Change</a></p><p>9 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 style="white-space: pre-wrap; word-wrap: break-word;">better to use a different name, because 'virtual' is a C++ keyword... virt or virtual_trunk or ...<br>If only to avoid confusing code highlighting, like here in gerrit.</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 style="white-space: pre-wrap; word-wrap: break-word;">(unrelated whitespace change)</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 style="white-space: pre-wrap; word-wrap: break-word;">(naming: first_endp_id or first_endp_nr? "offset" to me indicates that we are skipping some items in an array or data buffer, so for a couple minutes i was reading this code wrong. This variable only affects the endpoint id / number starting with at the start of the array, without an actual data offset. maybe it's just me though.)</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 style="white-space: pre-wrap; word-wrap: break-word;">'initialization'</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 style="white-space: pre-wrap; word-wrap: break-word;">Note that the vty allows vty_number_endpoints to be 0, in which case we probably should not invoke talloc?<br>Seems odd to allow zero endpoints, so see comment in mgcp_vty.c...</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 style="white-space: pre-wrap; word-wrap: break-word;">(separate issue: seems sketchy to use an API function that starts with an underscore and that isn't documented. Why not just talloc_zero_array(ctx, struct mgcp_endpoint*, number_endpoints) -- the only difference seems the name shown in the talloc report, right?)</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 style="white-space: pre-wrap; word-wrap: break-word;">...this is where vty_number_endpoints == 0 is allowed by the vty.<br>I think the problem could be fixed trivially by a separate patch changing this to '<1-65534>'.<br>(Only if we're reasonably sure that no current config out there sanely uses 0 in the cfg file)</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 style="white-space: pre-wrap; word-wrap: break-word;">Preferably add new vty commands in a separate patch, because the commit log says "refactor"</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 style="white-space: pre-wrap; word-wrap: break-word;">explain whether the used E1 timeslots end up being [first .. first+num] or [first .. num] ?</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: 2 </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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 06 Jul 2020 14:22:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>