<p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19075">View Change</a></p><p>4 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/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c">File src/libosmo-mgcp-client/mgcp_client.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/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@909">Patch Set #7, Line 909:</a> <code style="font-family:monospace,monospace">e1-0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the example says "e1-0" but trunk_id is <1-64>?</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/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@915">Patch Set #7, Line 915:</a> <code style="font-family:monospace,monospace">     static const uint8_t valid_offsets[] = { 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;">like last time, the const ones don't need to be static, or rather they should be further up in the .c file as global consts, and also it looks like there already are const arrays with the same numbers that this is code dup for</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/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@917">Patch Set #7, Line 917:</a> <code style="font-family:monospace,monospace">       static char endpoint[MGCP_ENDPOINT_MAXLEN];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">recently we're moving away from static char buffers and favor functions that write to a target buffer, or ones that use a talloc ctx to return talloc_asprinft() string.</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/+/19075/7/src/libosmo-mgcp-client/mgcp_client.c@931">Patch Set #7, Line 931:</a> <code style="font-family:monospace,monospace">             return NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">it seems odd to do this validation in a function that provides a string from numbers.<br>Consider that you'd like to log an endpoint that is invalid, this function then won't produce a string for it.<br>It might be good to be able to output:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  Invalid endpoint: ds/e1-1/s-0/su99-17</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Instead of</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  Cannot compose MGCP e1-endpoint name, E1-timeslot number (0) is invalid!<br>  Invalid endpoint: NULL</pre><p style="white-space: pre-wrap; word-wrap: break-word;">or in other words only produce the "invalid" error logs at a point where we are taking in parameters from the user and explicitly validate them.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(now I see laforge said something similar in an earlier patch set)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/19075">change 19075</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/+/19075"/><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: Iec35b5bae8a7b07ddb3559f7114a24dcd10e8f14 </div>
<div style="display:none"> Gerrit-Change-Number: 19075 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 09 Jul 2020 00:53:46 +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>