<p style="white-space: pre-wrap; word-wrap: break-word;">@neels: Thanks for commenting on this patch.</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372">View Change</a></p><p>21 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/+/18372/7//COMMIT_MSG">Commit Message:</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/+/18372/7//COMMIT_MSG@21">Patch Set #7, Line 21:</a> <code style="font-family:monospace,monospace">   symbol name "tcfg" to "trunk" in order to better match the reality.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(maybe do renames in a separate patch)</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/+/18372/7//COMMIT_MSG@28">Patch Set #7, Line 28:</a> <code style="font-family:monospace,monospace">   longer allocate them per trunk. Allocate them globally instead.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm wondering whether anyone would miss per-trunk counters in the future. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">up to now osmo-mgw was only used with the virtual trunk since E1 trunks never worked. There is already a discussion going on whether we should have the counters per trunk or not.</p><p style="white-space: pre-wrap; word-wrap: break-word;">See also: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/6//COMMIT_MSG#28</p><p style="white-space: pre-wrap; word-wrap: break-word;">We probably might have a closer look and see for which counters it would make sense to have them per trunk.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/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/+/18372/7/include/osmocom/mgcp/mgcp.h@a189">Patch Set #7, Line 189:</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;">char *audio_name;<br>        int audio_payload;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">these are no longer present in struct mgcp_trunk, maybe explain why in the commit log?</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/+/18372/7/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/+/18372/7/include/osmocom/mgcp/mgcp_trunk.h@38">Patch Set #7, Line 38:</a> <code style="font-family:monospace,monospace">       struct mgcp_endpoint **endpoints;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">changing this from 'struct mgcp_endpoint*' to 'struct mgcp_endpoint**'. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I just changed the array to a pointer array and allocate the the endpoints dynamically. This allows me to have a mgcp_endp_alloc() which can allocate everything an endpoint needs. I find it more logical when there is an alloc function for the endp that takes care of everything, including the memory.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However, the alternative would be to have some mgcp_endp_init() and call that for each array element but this approach looks less clean to me.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_codec.c">File src/libosmo-mgcp/mgcp_codec.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/+/18372/7/src/libosmo-mgcp/mgcp_codec.c@a289">Patch Set #7, Line 289:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this removal is not mentioned in the commit log. […]</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/+/18372/7/src/libosmo-mgcp/mgcp_conn.c">File src/libosmo-mgcp/mgcp_conn.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/+/18372/7/src/libosmo-mgcp/mgcp_conn.c@259">Patch Set #7, Line 259:</a> <code style="font-family:monospace,monospace">aggregate_rtp_conn_stats(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn_rtp)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This function does not access the individual endp, but accesses the single global struct mgcp_ratect […]</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/+/18372/3/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/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@100">Patch Set #3, Line 100:</a> <code style="font-family:monospace,monospace">         if (strlen(epname) <= prefix_len)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">so why not simply using strcmp() if they are both guaranteed to be null terminated?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am not sure, but I think it was correct in the beginning. What the function does is to return a pointer advanced to the position where the endpoint name begins.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The check strlen(epname) <= prefix_len is not necessary since strncmp will also return a value other than 0 when the strings have different length.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have to use strncmp because I only want to check the prefix, the epname sting usually contains the prefix and then some endpoint name. The endpoint name has to be omitted for the check of course.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Since I am confused now I have done an experiment, AAA is the prefix, AAABBB is the endpoint name with the prefix in front:</p><p style="white-space: pre-wrap; word-wrap: break-word;">This prints 1:<br>printf("%u\n",strcmp("AAABBB", "AAA"));</p><p style="white-space: pre-wrap; word-wrap: break-word;">This prints 0:<br>printf("%u\n",strncmp("AAABBB", "AAA", 3));</p><p style="white-space: pre-wrap; word-wrap: break-word;">(also the tests fail when I use strcmp ;-)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@90">Patch Set #7, Line 90:</a> <code style="font-family:monospace,monospace">/* Check if the endpoint name contains the prefix, and chop it off, if it</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(would be nice to include an example string for a prefix to make it easier to understand for uninfor […]</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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@140">Patch Set #7, Line 140:</a> <code style="font-family:monospace,monospace"> *  \param[out] cause, pointer to store cause code, can be NULL.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(I think doxygen wants no comma after 'cause'?)</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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@141">Patch Set #7, Line 141:</a> <code style="font-family:monospace,monospace"> *  \param[in] epname endpoint name to lookup (may lack trunk prefix and domain name).</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">wildcard should be explained</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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@164">Patch Set #7, Line 164:</a> <code style="font-family:monospace,monospace">    if (strncmp(epname_ch, "*", epname_ch_len) == 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If I get this right, with a full name, this function finds an existing (used?) endpoint. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">When we get a full endpoint name (one that is not wildcarded such as rtpbridge/1@mgw) we search for that specifc endpoint. If we do not find it we return NULL</p><p style="white-space: pre-wrap; word-wrap: break-word;">If the endpoint is part of a wildcarded request (e.g. rtpbridge/*@mgw) then we search for the next free endpoint and return that one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have now cleaned up the function a bit and also splitted the wildcarded part and the non wildcarded part. However, it still needs to come together in one function. The idea behind this to agregate the evaluation of the endpoint name at one central point.</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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@184">Patch Set #7, Line 184:</a> <code style="font-family:monospace,monospace">  /* Find an enspoint by its name (if wildcarded request is not</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">("enspoint")</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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@201">Patch Set #7, Line 201:</a> <code style="font-family:monospace,monospace">           trunk->trunk_nr, epname);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(might be better to leave the logging to callers. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I do not think that this would change much. I think its ok to log here, this way it is centralized and the caller does not have to care about it.</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/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@214">Patch Set #7, Line 214:</a> <code style="font-family:monospace,monospace">    domain_to_check = strstr(epname, "@");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(could be nice for the future to keep a single implementation for separating epname from domain, may […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this is ok, I also removed epname_len() now.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_network.c">File src/libosmo-mgcp/mgcp_network.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/+/18372/7/src/libosmo-mgcp/mgcp_network.c@1439">Patch Set #7, Line 1439:</a> <code style="font-family:monospace,monospace">               struct mgcp_rtp_end *rtp_end, struct mgcp_endpoint *endp)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">endp is only used for logging. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unfortunately there is no way to determine the endp with the rtp_end, there is no backpointer or something.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I see also that endp is only used for logging, but if we get rid of it we have no logging reference.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/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/+/18372/7/src/libosmo-mgcp/mgcp_trunk.c@77">Patch Set #7, Line 77:</a> <code style="font-family:monospace,monospace">                talloc_free(trunk->endpoints);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">the comment says "(re)allocate", but free+alloc loses all endpoint data. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">mgcp_trunk_alloc_endpts() is called once on startup when the VTY parses the config file. It is not intended to be called multiple times. I have now added an OSMO_ASSERT to ensure one can not call it multiple times.</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/+/18372/7/src/libosmo-mgcp/mgcp_trunk.c@79">Patch Set #7, Line 79:</a> <code style="font-family:monospace,monospace">                                            sizeof(struct mgcp_endpoint *), trunk->vty_number_endpoints, "endpoints");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">before this, maybe we should validate vty_number_endpoints > 0 (and maybe smaller than some sane max […]</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/+/18372/7/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/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@597">Patch Set #7, Line 597:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_mgcp_sdp_payload_number,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">DEFUN_DEPRECATED</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/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@610">Patch Set #7, Line 610:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_mgcp_sdp_payload_name,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">DEFUN_DEPRECATED</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/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@899">Patch Set #7, Line 899:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_trunk_payload_number,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">DEFUN_DEPRECATED</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/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@911">Patch Set #7, Line 911:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_trunk_payload_name,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">DEFUN_DEPRECATED</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/+/18372">change 18372</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/+/18372"/><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: Ice8aaf03faa2fd99074f8665eea3a696d30c5eb3 </div>
<div style="display:none"> Gerrit-Change-Number: 18372 </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: 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: Wed, 03 Jun 2020 14:11:07 +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: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>