<p style="white-space: pre-wrap; word-wrap: break-word;">Take a look if any of my remarks make sense, but none of this blocks.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I agree with laforge that keeping stats per-trunk is more desirable and also seems to be very easy here, just put a struct mgcp_ratectr in the trunk instead of the cfg? What's the rationale for having only global stats?</p><p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18644">View Change</a></p><p>13 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/+/18644/6/include/osmocom/mgcp/mgcp.h">File include/osmocom/mgcp/mgcp.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/+/18644/6/include/osmocom/mgcp/mgcp.h@251">Patch Set #6, Line 251:</a> <code style="font-family:monospace,monospace"> * health */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(it seems to be your personal style to prefer vertical comments, so I won't complain.<br>But let me say here once, me personally, I'd prefer using the 120 char width we have agreed on -- especially if it's a new line for only one word.)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@49">Patch Set #6, Line 49:</a> <code style="font-family:monospace,monospace"> endp->cfg = trunk->cfg;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not sure if whitespace at start of this line is correct.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I don't see anything wrong)</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@93">Patch Set #6, Line 93:</a> <code style="font-family:monospace,monospace"> * the same for all endpoints, so no ambiguity is introduced) */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would be nice to define "chop it off": write the epname without the prefix back to the memory pointed at by epname.</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@113">Patch Set #6, Line 113:</a> <code style="font-family:monospace,monospace"> return;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> default:<br> OSMO_ASSERT(false);</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@118">Patch Set #6, Line 118:</a> <code style="font-family:monospace,monospace"> * it off */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">also would be nice to define "chop it off" here: truncate epname by writing a '\0' char where the suffix starts.</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@136">Patch Set #6, Line 136:</a> <code style="font-family:monospace,monospace"> * and not needed to identify the endpoint on the trunk */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't understand "all information that is already evaluated".<br>Maybe "return the epname stripped of prefix and suffix and converted to lower case"?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also would be nice to indicate the expected buf size of epname_stripped.</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@140">Patch Set #6, Line 140:</a> <code style="font-family:monospace,monospace"> osmo_str_tolower_buf(epname_stripped, strlen(epname), epname);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the second argument is the memory length of epname_stripped to safeguard against writing past its end, regardless of the input strlen. Passing strlen(epname) is breaking the purpose of that len. Since there is no explicit epname_stripped_buflen arg, IIUC this should be MGCP_ENDPOINT_MAXLEN?</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@181">Patch Set #6, Line 181:</a> <code style="font-family:monospace,monospace"> endp->wildcarded_req = false;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this function is called "find_...", could be a bad idea to also modify the endp here?<br>Though I must admit that I'm not sure what wildcarded_req does.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(then you could also add const to the 'trunk' arg)</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@195">Patch Set #6, Line 195:</a> <code style="font-family:monospace,monospace"> struct mgcp_trunk *trunk)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const struct mgcp_trunk ?</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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@210">Patch Set #6, Line 210:</a> <code style="font-family:monospace,monospace"> trunk->trunk_nr, endp->name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think set endp->wildcarded = true 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/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@225">Patch Set #6, Line 225:</a> <code style="font-family:monospace,monospace"> if (endp) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">and wildcarded = false here?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_msg.c">File src/libosmo-mgcp/mgcp_msg.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/+/18644/6/src/libosmo-mgcp/mgcp_msg.c@234">Patch Set #6, Line 234:</a> <code style="font-family:monospace,monospace"> line, endp->name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">very good to see the "0x1" endp naming go away!</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/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/+/18644/6/src/libosmo-mgcp/mgcp_trunk.c@143">Patch Set #6, Line 143:</a> <code style="font-family:monospace,monospace">struct mgcp_trunk *mgcp_trunk_by_name(const char *epname, struct mgcp_config *cfg)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if this is a new function: the mgcp_trunk_by_num() has cfg as the first argument, better also do that here.<br>(If this is just moving an old function, then rather keep it inconsistent I guess.)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18644">change 18644</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/+/18644"/><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: Ia8cf4d6caf05a4e13f1f507dc68cbabb7e6239aa </div>
<div style="display:none"> Gerrit-Change-Number: 18644 </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: Wed, 10 Jun 2020 19:41:40 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>