<p><a href="https://gerrit.osmocom.org/11521">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11521/1/src/libosmo-mgcp/mgcp_protocol.c">File src/libosmo-mgcp/mgcp_protocol.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/11521/1/src/libosmo-mgcp/mgcp_protocol.c@1573">Patch Set #1, Line 1573:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Fine, so it seems Harald has changed his mind, or I misunderstood when we talked months ago. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I like to use OSMO_ASSERT() on allocations, because if we cannot allocate a struct, how can we expect to allocate a string buffer / a gsmtap-logging packet /... to report that error in the first place? If we could not allocate, we are anyway shagged and might as well give up, no need to compose error message strings in the land of doom. That's what I thought.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c">File src/libosmo-mgcp/mgcp_protocol.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/11521/4/src/libosmo-mgcp/mgcp_protocol.c@111">Patch Set #4, Line 111:</a> <code style="font-family:monospace,monospace">   [MGCP_DLCX_FAIL_INVALID_CALLID] = {"dlcx:callid", "invalid CallId specified in DLCX command."},</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">reading the code below I think this is "DLCX error: specified CallId mismatches the endpoint's CallId"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@112">Patch Set #4, Line 112:</a> <code style="font-family:monospace,monospace">   [MGCP_DLCX_FAIL_INVALID_CONNID] = {"dlcx:connid", "invalid connection ID specified in DLCX command."},</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">and this is "DLCX error: specified Connection ID does not exist on the endpoint"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@116">Patch Set #4, Line 116:</a> <code style="font-family:monospace,monospace">      [MGCP_DLCX_FAIL_NO_RTP] = {"dlcx:no_rtp", "no rtp stream associated with connection."},</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I have checked this. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Almost, but you mixed up CallId with ConnId. CallId is just a number e.g. the MSC invented to identify this voice call. The ConnId is the CI hexstring that identifies one connection on an endpoint, and the ConnId is used to DLCX one conn of an endpoint. TLDR: this is related to ConnId.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, so below code, when reaching line 1371, has already found an endpoint with the given name. But then the ConnId passed in the DLCX cannot be found on that endpoint by mgcp_conn_get_rtp(). The same error should actually already hit in line 1305 below, where we use mgcp_verify_ci() to see that the ConnId is valid *and exists*. So I think we can drop FAIL_NO_RTP and use INVALID_CONNID instead.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I still think in practice we would never hit the error at 1371. The only condition leading to hitting an error there would be conn->type != MGCP_CONN_TYPE_RTP. What conn types do we have besides RTP?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">    enum mgcp_conn_type {<br>            MGCP_CONN_TYPE_RTP,<br>    };</pre><p style="white-space: pre-wrap; word-wrap: break-word;">...as I suspected</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@1546">Patch Set #4, Line 1546:</a> <code style="font-family:monospace,monospace">         trunk->mgcp_crcx_ctr_group = rate_ctr_group_alloc(ctx, &mgcp_crcx_ctr_group_desc, crcx_rate_ctr_index);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm sorry to say it, but, though the OSMO_ASSERT() discussion was a valid one to have, it was more or less displaced for rate_ctr_group_alloc() in particular, because that one returns NULL for name errors, and apparently no-one noticed that rather important trait before.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But I see that other code around this also makes the same mistake of not checking the result of rate_ctr_group_alloc(), so I would agree to fix all of those *in a separate patch*: https://osmocom.org/issues/3701</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11521">change 11521</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/11521"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b </div>
<div style="display:none"> Gerrit-Change-Number: 11521 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 20 Nov 2018 01:33:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>