<p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25008">View Change</a></p><p>9 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">seems to me that it is not necessary to copy this list. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The list is copied because osmo-mgcp-client is designed that way. The vty configuration is separated from the client itsself. When the client is started it copies all configuration items. Doing it differently would break this pattern. We also cannot just copy the llist head. I think it is better to keep it this way.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">hm, interesting! I think we should change mgcp_client_tx() to still store an entry […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have checked this back now. Unfortunately we cannot change this because there is a bug that whould lead into a memory leak, see change id: I83938ff47fa8570b8d9dc810a184864a0c0b58aa</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't really know how to fix this, but probably we would need some garbage collector that deletes entries from the queue when there is no response from some time. I think this problem is not only triggered by mgcp_client_fsm, but may also easily occur when someone is using the bare mgcp-client without its fsm extensions. This would require a separate patch/ticket. My idea would be just having a timestamp in each queue entry and when there is no response after a minute or so it would delete it from the queue.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For now we could remove the dummy function and live with the error message or just keep it the way it ist. It does not hurt.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think it is more elegant to define the complete endpoint name in the config, […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is intentional. There is already a way to specify the domain name in the config. If you would allow to specify the domain name again in this reset endpoint feature than you might get an invalid configuration. The configuration would also be redundant since the domain name is already specified at a different point in the config.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It makes no sense to configure a reset endpoint rtpbridge/*@bsc when the domain name is configured as @mgw. The MGW would reject rtpbridge/*@bsc because it would expect the domain name to be @mgw.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(x) mentioned above, here remove the response_cb != NULL condition. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(see above, this works around a memory leak)</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"endpoint" […]</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/+/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(but i think it should be)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I see that differently, see my other comment)</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@188">Patch Set #4, Line 188:</a> <code style="font-family:monospace,monospace">   if (osmo_strlcpy(reset_ep->name, argv[0], sizeof(reset_ep->name)) >= sizeof(reset_ep->name)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">repeated osmo_strlcpy with line above... […]</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/+/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">("on connect" as above) […]</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/+/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">break; ?  or put the free() directly in the loop and return CMD_SUCCESS here?</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/+/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: 5 </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: Tue, 27 Jul 2021 10:01:27 +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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>