<p style="white-space: pre-wrap; word-wrap: break-word;">Nice, a lot of excellent cleanup work!</p><p style="white-space: pre-wrap; word-wrap: break-word;">I hope you see the large number of comments as appreciation of the patch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">May seem ironic from me as Mr. Code Bomb himself, but this patch would be fairly easy to split into smaller parts. I'm thinking the deprecation of the sdp audio name vty commands, some renames, and ideally moving old code to new files first without changing them so that code review can easily see the changes made to the code?</p><p style="white-space: pre-wrap; word-wrap: break-word;">The -1 vote is for the DEFUN_DEPRECATED, and missing items in the commit log (or alternatively splitting up the patch), and the talloc_free of trunk->endpoints.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Anyway, cool to see osmo-mgw being streamlined :)</p><p>Patch set 7:<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/+/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 style="white-space: pre-wrap; word-wrap: break-word;">(maybe do renames in a separate patch)</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 style="white-space: pre-wrap; word-wrap: break-word;">I'm wondering whether anyone would miss per-trunk counters in the future. I don't know of anyone using more than one trunk, but if we have multiple trunks supported in the code, maybe per-trunk counters would be nicer to keep than to remove?</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 style="white-space: pre-wrap; word-wrap: break-word;">these are no longer present in struct mgcp_trunk, maybe explain why in the commit log?</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 style="white-space: pre-wrap; word-wrap: break-word;">changing this from 'struct mgcp_endpoint*' to 'struct mgcp_endpoint**'.<br>What is the reason? If I see this right, we are anyway just allocating a fixed number of endpoints at trunk creation time, and it seems unnecessary to add to this the individual dynamic allocation for every contained endpoint?</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 style="white-space: pre-wrap; word-wrap: break-word;">this removal is not mentioned in the commit log. I guess it should be a separate patch from the refactoring.</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 style="white-space: pre-wrap; word-wrap: break-word;">This function does not access the individual endp, but accesses the single global struct mgcp_ratectr. It would make more sense to pass a struct mgcp_ratectr as argument, to reflect the fact that now there is only one global set of counters (besides the per-conn counters).</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 style="white-space: pre-wrap; word-wrap: break-word;">(would be nice to include an example string for a prefix to make it easier to understand for uninformed readers)</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 style="white-space: pre-wrap; word-wrap: break-word;">(I think doxygen wants no comma after 'cause'?)</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 style="white-space: pre-wrap; word-wrap: break-word;">wildcard should be explained</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 style="white-space: pre-wrap; word-wrap: break-word;">If I get this right, with a full name, this function finds an existing (used?) endpoint.<br>In this condition here, if the endpoint name is exactly "*", this finds the first unused endpoint.<br>It feels to me like these should be two separate functions, one takes an epname to look up, the other takes no epname and finds an unused one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">EDIT: I see now that this function was just moved to a different 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/+/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 style="white-space: pre-wrap; word-wrap: break-word;">("enspoint")</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 style="white-space: pre-wrap; word-wrap: break-word;">(might be better to leave the logging to callers. There could be cases where a caller expects that some endpoint should be gone, and then an error log of "Not able to find" would confuse readers of the osmo-mgw log to think that something went wrong, even though all is as expected)</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 style="white-space: pre-wrap; word-wrap: break-word;">(could be nice for the future to keep a single implementation for separating epname from domain, maybe use epname_len(), so we have only one place that does strchr('@')? Just an idea...)</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 style="white-space: pre-wrap; word-wrap: break-word;">endp is only used for logging. Isn't rtp_end also part of endp? Maybe I'm wrong, but seems like it should be enough to pass only endp or only rtp_end?</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 style="white-space: pre-wrap; word-wrap: break-word;">the comment says "(re)allocate", but free+alloc loses all endpoint data.<br>Is this intended to change during a running osmo-mgw without losing endpoint state?<br>Then we could use talloc_realloc() instead, which keeps the data (as much of it as is possible).</p><p style="white-space: pre-wrap; word-wrap: break-word;">If not, then talloc_free(trunk->endpoints) might not be necessary, or should maybe also first take care to shut down RTP ports and so on?</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 style="white-space: pre-wrap; word-wrap: break-word;">before this, maybe we should validate vty_number_endpoints > 0 (and maybe smaller than some sane maximum?)</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@a117">Patch Set #7, Line 117:</a> <code style="font-family:monospace,monospace">             vty_out(vty, "  sdp audio-payload number %d%s",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">not mentioned in the commit log?</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@597">Patch Set #7, Line 597:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_mgcp_sdp_payload_number,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">DEFUN_DEPRECATED</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 style="white-space: pre-wrap; word-wrap: break-word;">DEFUN_DEPRECATED</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 style="white-space: pre-wrap; word-wrap: break-word;">DEFUN_DEPRECATED</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 style="white-space: pre-wrap; word-wrap: break-word;">DEFUN_DEPRECATED</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: Tue, 02 Jun 2020 13:54:45 +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>