Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11521 )
Change subject: add DLCX command statistics to osmo-mgw ...................................................................... Patch Set 4: (5 comments) https://gerrit.osmocom.org/#/c/11521/1/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/#/c/11521/1/src/libosmo-mgcp/mgcp_protocol.c@1573 PS1, Line 1573: > Fine, so it seems Harald has changed his mind, or I misunderstood when we > talked months ago. […] 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. https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@111 PS4, Line 111: [MGCP_DLCX_FAIL_INVALID_CALLID] = {"dlcx:callid", "invalid CallId specified in DLCX command."}, reading the code below I think this is "DLCX error: specified CallId mismatches the endpoint's CallId" https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@112 PS4, Line 112: [MGCP_DLCX_FAIL_INVALID_CONNID] = {"dlcx:connid", "invalid connection ID specified in DLCX command."}, and this is "DLCX error: specified Connection ID does not exist on the endpoint" https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@116 PS4, Line 116: [MGCP_DLCX_FAIL_NO_RTP] = {"dlcx:no_rtp", "no rtp stream associated with connection."}, > I have checked this. […] 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. 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. 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? enum mgcp_conn_type { MGCP_CONN_TYPE_RTP, }; ...as I suspected https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@1546 PS4, Line 1546: trunk->mgcp_crcx_ctr_group = rate_ctr_group_alloc(ctx, &mgcp_crcx_ctr_group_desc, crcx_rate_ctr_index); 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. 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 -- To view, visit https://gerrit.osmocom.org/11521 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Gerrit-Change-Number: 11521 Gerrit-PatchSet: 4 Gerrit-Owner: Stefan Sperling <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Stefan Sperling <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-Comment-Date: Tue, 20 Nov 2018 01:33:34 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
