<p style="white-space: pre-wrap; word-wrap: break-word;">This change is ready for review.</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18881">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18881/5/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/+/18881/5/src/libosmo-mgcp-client/mgcp_client.c@1002">Patch Set #5, Line 1002:</a> <code style="font-family:monospace,monospace">int mgcp_client_cancel(struct mgcp_client *mgcp, mgcp_trans_id_t trans_id)</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">it looks like osmo-bsc fails to call mgcp_client_cancel() in some situation, and the timeout or teardown should happen in osmo-bsc?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">As can be seen from the header file name (mgcp_client_internal.h), this is an internal API. Thus osmo-bsc is not supposed to call it. It's only used here, in this file, and by the FSM implementation (mgcp_client_fsm.c).</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">to identify the code path in osmo-bsc that might fail to call mgcp_client_cancel().</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">See above, osmo-bsc has no access to this API.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I'm particularly 'worried' about that comment saying "the FSM likely already terminated, thus we don't call osmo_fsm_inst_term()"</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, the point is that a (potentially leaked) message is enqueued by an MGCP FSM instance _during termination_. Therefore nobody is responsible for tracking stalled messages in the queue. Normally the FSM itself handles response timeout and calls mgcp_client_cancel(). This is what I tried to explain in the commit message, perhaps I failed to make it easy to read/understand.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">this patch is inviting trouble.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">This patch solves the problem. At least I don't see leaked chunks after each test case anymore. Yes, we can refactor everything, or simply do not enqueue such ownerless messages, but this would require more efforts to achieve the same goal - fix memleak.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I am not against making this implementation better, feel free to do so if you want. But personally I don't want (nor can) spend even more time on this. It already took too much.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/18881">change 18881</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/+/18881"/><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: Ia2e89f31685a0822e5cb147a06cc1fc68efc1ec4 </div>
<div style="display:none"> Gerrit-Change-Number: 18881 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: fixeria <vyanitskiy@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: fixeria <vyanitskiy@sysmocom.de> </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-Comment-Date: Thu, 18 Jun 2020 22:35:11 +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"> Gerrit-MessageType: comment </div>