<p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25008">View Change</a></p><p>8 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/+/25008/4/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/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@793">Patch Set #4, Line 793:</a> <code style="font-family:monospace,monospace">            llist_add_tail(&actual_reset_ep->list, &mgcp->actual.reset_epnames);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">seems to me that it is not necessary to copy this list.<br>IIUC the list is only used once at program startup, just after reading in the cfg.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Would it also work to just use conf->reset_epnames on connect?<br>that way, a client that re-connects without restarting the program<br>would also start using changes made to the list by telnet VTY immediately.</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/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@854">Patch Set #4, Line 854:</a> <code style="font-family:monospace,monospace">void _ignore_mgcp_response(struct mgcp_response *response, void *priv) { }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">hm, interesting! I think we should change mgcp_client_tx() to still store an entry<br>via mgcp_client_pending_add() even if response_cb is NULL, see (x) below.</p><p style="white-space: pre-wrap; word-wrap: break-word;">That way we can pass NULL to ignore the response and don't get an error log for it,<br>and don't need to define an empty function.</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/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@905">Patch Set #4, Line 905:</a> <code style="font-family:monospace,monospace">             epname = _mgcp_client_name_append_domain(mgcp, reset_ep->name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it is more elegant to define the complete endpoint name in the config,<br>and to not append a domain here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For example, I'm the admin of an osmo-mgw, and the domain configured in osmo-mgw.cfg was "@mgw",<br>and say I'm now changing it to "@bsc".<br>The next time the client restarts, I still want to make sure the legacy domain name also gets reset.<br>So I might want to add both</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  rtpbridge/*@mgw<br>  rtpbridge/*@bsc</pre><p style="white-space: pre-wrap; word-wrap: break-word;">If you hardcode here that a domain is still added internally, you are taking this<br>configuration freedom away from the user.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I think the solution is to simply not append a domain name,<br>and document that the cfg must also contain the @domain part.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If you think it is important to be able to omit the domain name in the config,<br>then let's only add a domain name here when there is no '@' in the name yet?<br>i.e. only add a domain when it is missing, so that the user can decide?</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/+/25008/4/src/libosmo-mgcp-client/mgcp_client.c@1052">Patch Set #4, Line 1052:</a> <code style="font-family:monospace,monospace">     if (response_cb != NULL) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(x) mentioned above, here remove the response_cb != NULL condition.<br>That's all that's needed, because we already avoid calling a NULL response_cb in mgcp_client_handle_response().</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c">File src/libosmo-mgcp-client/mgcp_client_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/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@161">Patch Set #4, Line 161:</a> <code style="font-family:monospace,monospace">eindpoint</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"endpoint"</p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's write "Add" instead of "Set", so it's clear that there can be more than one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's write "on connect" instead of "startup", because it's actually sent in mgcp_client_connect()</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/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@175">Patch Set #4, Line 175:</a> <code style="font-family:monospace,monospace">        /* the domain name is not part of the actual endpoint name */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(but i think it should be)</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/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@202">Patch Set #4, Line 202:</a> <code style="font-family:monospace,monospace">startup</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">("on connect" as above)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's write "Remove an endpoint name from the reset-endpoint list"</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/+/25008/4/src/libosmo-mgcp-client/mgcp_client_vty.c@210">Patch Set #4, Line 210:</a> <code style="font-family:monospace,monospace">                      reset_ep_del = reset_ep;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">break; ?  or put the free() directly in the loop and return CMD_SUCCESS here?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25008">change 25008</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/+/25008"/><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: I47e7ff858d5067b46d52329be5f362ff61c0dff8 </div>
<div style="display:none"> Gerrit-Change-Number: 25008 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </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: daniel <dwillmann@sysmocom.de> </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: Sat, 24 Jul 2021 23:06:41 +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>