Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Tue Nov 20 01:33:34 UTC 2018


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 <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Comment-Date: Tue, 20 Nov 2018 01:33:34 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181120/b63e17d6/attachment.htm>


More information about the gerrit-log mailing list