<p style="white-space: pre-wrap; word-wrap: break-word;">Very readable! Some thoughts:</p><ul><li>since this changes the VTY logic a lot, having VTY tests would be nice. from a quick glance it looks like there aren't any yet for mgcp-client, so not sure if this is reasonable.</li><li>adjust / add an example config?</li></ul><p>Patch set 5:<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/+/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 style="white-space: pre-wrap; word-wrap: break-word;">that</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 style="white-space: pre-wrap; word-wrap: break-word;">Adding the retry_n_ports and other slight tweaks to this function seem like a different logical change. Maybe put this into a separate commit?</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 style="white-space: pre-wrap; word-wrap: break-word;">client</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 style="white-space: pre-wrap; word-wrap: break-word;">initialization</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 style="white-space: pre-wrap; word-wrap: break-word;">maybe it's just me, but I find this a bit misleading: it sounds like the mgcp client was taken "out of the pool", as in, it can't be used anymore. but what happened earlier is, that it was picked and got a call assigned, the ref counter was increased. it will still be used if all other mgcp clients swimming in the pool have a higher refcount.</p><p style="white-space: pre-wrap; word-wrap: break-word;">but that's more of a nitpick, all in all I find this patch and newly introduced code very readable.</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 style="white-space: pre-wrap; word-wrap: break-word;">this comment describes what the whole function does, so I'd merge it with the function description on top of the function and put it there</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><pre style="font-family: monospace,monospace; white-space: pre-wrap;">just to be sure, is this intended? I'm wondering if vty->index has type<br>  struct mgcp_client_conf</pre></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 style="white-space: pre-wrap; word-wrap: break-word;">\ at the end of the macro looks wrong</p><p style="white-space: pre-wrap; word-wrap: break-word;">(also, being curious: what's the advantage of using a macro here?)</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 style="white-space: pre-wrap; word-wrap: break-word;">just to be sure: is this extra space on purpose? given that there's already two +1 above for indent_buf_len</p><p style="white-space: pre-wrap; word-wrap: break-word;">the whole indent logic here seems a bit complicated TBH, is the global variable really necessary? it gets written to every time config_write_pool is called anyway.</p><p style="white-space: pre-wrap; word-wrap: break-word;">after looking at this some more, it seems with each config_write_pool, the snprintf here tries to write more and more bytes after global_mgcp_client_indent_pool. that's certainly not what's intended? so if this variable is used, the passed size shouldn't be strlen + n, rather sizeof and a fixed buffer length?</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 style="white-space: pre-wrap; word-wrap: break-word;">(I guess for users it would be useful to have it block (not use it for new calls) and auto-remove the mgw once all calls are served - maybe that's useful for a follow-up 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/+/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 style="white-space: pre-wrap; word-wrap: break-word;">with the check below it won't drop ongoing calls</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 style="white-space: pre-wrap; word-wrap: break-word;">shouldn't this be talloc_strdup or something? but even better would be to get rid of the global var if possible IMHO, see my other comment</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: 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: 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 09:42:48 +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>