<p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for the review.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In order to get VTY tests I think we would need an application first. This could be a dummy application maybe or we could add some checks to osmo-bsc where the client is used. However the VTY for the MGCP Client is not very complex. At least for this patch I think I did not damage anything.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also in the osmo-mgw we do not have any example configs for the client. In osmo-bsc I could add an example config but I don't know if this makes sense since I have already put an example into the manual. See also: osmo-bsc I8f33ab2cea04b545c403a6fe479aa963a0fc0d0d</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25120">View Change</a></p><p>12 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/+/25120/3/include/osmocom/mgcp_client/mgcp_client_pool_internal.h">File include/osmocom/mgcp_client/mgcp_client_pool_internal.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/+/25120/3/include/osmocom/mgcp_client/mgcp_client_pool_internal.h@21">Patch Set #3, Line 21:</a> <code style="font-family:monospace,monospace">thet</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">that</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/+/25120/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/+/25120/4/src/libosmo-mgcp-client/mgcp_client.c@799">Patch Set #4, Line 799:</a> <code style="font-family:monospace,monospace">static int init_socket(struct mgcp_client *mgcp, unsigned int retry_n_ports)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Adding the retry_n_ports and other slight tweaks to this function seem like a different logical chan […]</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c">File src/libosmo-mgcp-client/mgcp_client_pool.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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@43">Patch Set #5, Line 43:</a> <code style="font-family:monospace,monospace">clien</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">client</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@56">Patch Set #5, Line 56:</a> <code style="font-family:monospace,monospace">initalization</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">initialization</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@153">Patch Set #5, Line 153:</a> <code style="font-family:monospace,monospace">put an MGCP client back into the pool</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">maybe it's just me, but I find this a bit misleading: it sounds like the mgcp client was taken "out  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes it sounds a bit like that but I do not know how to explain it differently. I have added some more comments to make clear what actually happens - so nobody gets confused.</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_pool.c@159">Patch Set #5, Line 159:</a> <code style="font-family:monospace,monospace">     *  not belong to a pool at all, the function call will have no effect. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this comment describes what the whole function does, so I'd merge it with the function description o […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think that is done automatically by doxygen. I have marked the comment with a /*! that should work.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25120/5/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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@47">Patch Set #5, Line 47:</a> <code style="font-family:monospace,monospace">vty->index</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">just to be sure, is this intended? I'm wondering if vty->index has type […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It should be correct. Index is void pointer. See cfg_mgw_cmd, there I am assigning the configuration.</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@50">Patch Set #5, Line 50:</a> <code style="font-family:monospace,monospace"> \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">\ at the end of the macro looks wrong […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have to use this macro in each (common) VTY command to get the pointer to the conf. There are quite a few of them so it reduces some code dup.</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@329">Patch Set #5, Line 329:</a> <code style="font-family:monospace,monospace"> </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">just to be sure: is this extra space on purpose? given that there's already two +1 above for indent_ […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The extra space is on purpose. I use it to indent the mgw vty settings under the mgw x node by one space. </p><p style="white-space: pre-wrap; word-wrap: break-word;">The global_mgcp_client_indent_pool should be written only once. I don't really see what you mean but I managed to eliminate the global variable.</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@394">Patch Set #5, Line 394:</a> <code style="font-family:monospace,monospace">                 pool_member->nr, VTY_NEWLINE);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(I guess for users it would be useful to have it block (not use it for new calls) and auto-remove th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it is better if blocking and removing is done consciously. It is also not done very often.</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@410">Patch Set #5, Line 410:</a> <code style="font-family:monospace,monospace">will drop ongoing calls</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">with the check below it won't drop ongoing calls</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/+/25120/5/src/libosmo-mgcp-client/mgcp_client_vty.c@531">Patch Set #5, Line 531:</a> <code style="font-family:monospace,monospace">indent</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">shouldn't this be talloc_strdup or something? but even better would be to get rid of the global var  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would normally expect the indent string to be a constant, but who knows where an API user is getting this from.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I had a closer look to the global variables and I have now eliminated as much of them as I could. At the moment this VTY implementation can only manage one pool. This is a limitation, but at the Moment we do not have any application that would benefit from multiple pools. However, I wonder if it would be possible to manage multiple pools but the only way to do it is to use a vty->index that is populated by the VTY of the application. But think that would be too complicated for now.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25120">change 25120</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/+/25120"/><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: Icaaba0e470e916eefddfee750b83f5f65291a6b0 </div>
<div style="display:none"> Gerrit-Change-Number: 25120 </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: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 04 Aug 2021 15:04:05 +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: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>