Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has submitted this change and it was merged. (
https://gerrit.osmocom.org/11521 )
Change subject: add DLCX command statistics to osmo-mgw
..
add DLCX command statistics to osmo-mgw
Add a counter group for DLCX commands. The group contains counters for
successful connection processing as well as various error conditions.
This provides a quick overview of DLCX failures on each trunk throughout
the lifetime of the osmo-mgw process.
The counters are displayed by 'show mgcp stats' and 'show rate-counters'
While here, rename MGCP_MDCX_FAIL_DEFERRED_BY_POLICY to
MGCP_MDCX_DEFERRED_BY_POLICY; we have decided that deferred connections
aren't failures, and this keeps names used by DLCX and MDCX in sync.
Also remove some allocation failure checks with OSMO_ASSERT(); such
checks aren't en vogue anymore.
Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b
Depends: I80d36181600901ae2e0f321dc02b5d54ddc94139I
Related: OS#2660
---
M include/osmocom/mgcp/mgcp.h
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_vty.c
3 files changed, 66 insertions(+), 11 deletions(-)
Approvals:
Neels Hofmeyr: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/include/osmocom/mgcp/mgcp.h b/include/osmocom/mgcp/mgcp.h
index aa5607f..034a780 100644
--- a/include/osmocom/mgcp/mgcp.h
+++ b/include/osmocom/mgcp/mgcp.h
@@ -152,9 +152,20 @@
MGCP_MDCX_FAIL_NO_REMOTE_CONN_DESC,
MGCP_MDCX_FAIL_START_RTP,
MGCP_MDCX_FAIL_REJECTED_BY_POLICY,
- MGCP_MDCX_FAIL_DEFERRED_BY_POLICY
+ MGCP_MDCX_DEFERRED_BY_POLICY
};
+/* Global MCGP DLCX related rate counters */
+enum {
+ MGCP_DLCX_SUCCESS,
+ MGCP_DLCX_FAIL_WILDCARD,
+ MGCP_DLCX_FAIL_NO_CONN,
+ MGCP_DLCX_FAIL_INVALID_CALLID,
+ MGCP_DLCX_FAIL_INVALID_CONNID,
+ MGCP_DLCX_FAIL_UNHANDLED_PARAM,
+ MGCP_DLCX_FAIL_REJECTED_BY_POLICY,
+ MGCP_DLCX_DEFERRED_BY_POLICY,
+};
struct mgcp_trunk_config {
struct llist_head entry;
@@ -198,6 +209,8 @@
struct rate_ctr_group *mgcp_crcx_ctr_group;
/* Rate counter group which contains stats for processed MDCX commands.
*/
struct rate_ctr_group *mgcp_mdcx_ctr_group;
+ /* Rate counter group which contains stats for processed DLCX commands.
*/
+ struct rate_ctr_group *mgcp_dlcx_ctr_group;
/* Rate counter group which aggregates stats of individual RTP
connections. */
struct rate_ctr_group *all_rtp_conn_stats;
};
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index 3313164..eaf0e5a 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -93,7 +93,7 @@
[MGCP_MDCX_FAIL_NO_REMOTE_CONN_DESC] = {"mdcx:no_remote_conn_desc", "no
opposite end specified for connection."},
[MGCP_MDCX_FAIL_START_RTP] = {"mdcx:start_rtp_failure", "failure to
start RTP processing."},
[MGCP_MDCX_FAIL_REJECTED_BY_POLICY] = {"mdcx:conn_rejected",
"connection rejected by policy."},
- [MGCP_MDCX_FAIL_DEFERRED_BY_POLICY] = {"mdcx:conn_deferred",
"connection deferred by policy."},
+ [MGCP_MDCX_DEFERRED_BY_POLICY] = {"mdcx:conn_deferred", "connection
deferred by policy."},
};
const static struct rate_ctr_group_desc mgcp_mdcx_ctr_group_desc = {
@@ -104,6 +104,25 @@
.ctr_desc = mgcp_mdcx_ctr_desc
};
+static const struct rate_ctr_desc mgcp_dlcx_ctr_desc[] = {
+ [MGCP_DLCX_SUCCESS] = {"dlcx:success", "DLCX command processed
successfully."},
+ [MGCP_DLCX_FAIL_WILDCARD] = {"dlcx:wildcard", "wildcard names in DLCX
commands are unsupported."},
+ [MGCP_DLCX_FAIL_NO_CONN] = {"dlcx:no_conn", "endpoint specified in DLCX
command has no active connections."},
+ [MGCP_DLCX_FAIL_INVALID_CALLID] = {"dlcx:callid", "CallId specified in
DLCX command mismatches endpoint's CallId ."},
+ [MGCP_DLCX_FAIL_INVALID_CONNID] = {"dlcx:connid", "connection ID
specified in DLCX command does not exist on endpoint."},
+ [MGCP_DLCX_FAIL_UNHANDLED_PARAM] = {"dlcx:unhandled_param", "unhandled
parameter in DLCX command."},
+ [MGCP_DLCX_FAIL_REJECTED_BY_POLICY] = {"dlcx:rejected", "connection
deletion rejected by policy."},
+ [MGCP_DLCX_DEFERRED_BY_POLICY] = {"dlcx:deferred", "connection deletion
deferred by policy."},
+};
+
+const static struct rate_ctr_group_desc mgcp_dlcx_ctr_group_desc = {
+ .group_name_prefix = "dlcx",
+ .group_description = "dlcx statistics",
+ .class_id = OSMO_STATS_CLASS_GLOBAL,
+ .num_ctr = ARRAY_SIZE(mgcp_dlcx_ctr_desc),
+ .ctr_desc = mgcp_dlcx_ctr_desc
+};
+
const static struct rate_ctr_group_desc all_rtp_conn_rate_ctr_group_desc = {
.group_name_prefix = "all_rtp_conn",
.group_description = "aggregated statistics for all rtp connections",
@@ -1192,7 +1211,7 @@
LOGP(DLMGCP, LOGL_DEBUG,
"MDCX: end
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 5: pourquoi tu ne merge pas? waiting for the patch to acquire a bouquet first? -- 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: 5 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: dexter Gerrit-Comment-Date: Tue, 27 Nov 2018 11:19:55 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 5: Code-Review+2 -- 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: 5 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: dexter Gerrit-Comment-Date: Fri, 23 Nov 2018 15:43:08 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. (
https://gerrit.osmocom.org/11521 )
Change subject: add DLCX command statistics to osmo-mgw
..
Patch Set 4:
(4 comments)
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 CallI […]
Changed string in next patch set.
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"
Changed string in next patch set.
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."},
> Almost, but you mixed up CallId with ConnId. CallId is just a number e.g. […]
We can still write this code as if more than one conn type existed, though.
That won't hurt.
I have dropped FAIL_NO_RTP from the next patch set, it will use
FAIL_INVALID_CONNID instead.
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 o […]
Yes, let's treat this as a separate problem.
--
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
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Pau Espin Pedrol
Gerrit-Reviewer: Stefan Sperling
Gerrit-Reviewer: dexter
Gerrit-Comment-Date: Tue, 20 Nov 2018 09:34:13 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Hello dexter, Pau Espin Pedrol, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11521 to look at the new patch set (#5). Change subject: add DLCX command statistics to osmo-mgw .. add DLCX command statistics to osmo-mgw Add a counter group for DLCX commands. The group contains counters for successful connection processing as well as various error conditions. This provides a quick overview of DLCX failures on each trunk throughout the lifetime of the osmo-mgw process. The counters are displayed by 'show mgcp stats' and 'show rate-counters' While here, rename MGCP_MDCX_FAIL_DEFERRED_BY_POLICY to MGCP_MDCX_DEFERRED_BY_POLICY; we have decided that deferred connections aren't failures, and this keeps names used by DLCX and MDCX in sync. Also remove some allocation failure checks with OSMO_ASSERT(); such checks aren't en vogue anymore. Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Depends: I80d36181600901ae2e0f321dc02b5d54ddc94139I Related: OS#2660 --- M include/osmocom/mgcp/mgcp.h M src/libosmo-mgcp/mgcp_protocol.c M src/libosmo-mgcp/mgcp_vty.c 3 files changed, 66 insertions(+), 11 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/21/11521/5 -- 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: newpatchset Gerrit-Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Gerrit-Change-Number: 11521 Gerrit-PatchSet: 5 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: dexter
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
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
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Pau Espin Pedrol
Gerrit-Reviewer: Stefan Sperling
Gerrit-Reviewer: dexter
Gerrit-Comment-Date: Tue, 20 Nov 2018 01:33:34 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
dexter has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 4: Code-Review+1 > (1 comment) > > > (1 comment) > > (making sure my comment gets posted) -- 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 Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: dexter Gerrit-Comment-Date: Mon, 19 Nov 2018 11:40:12 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
dexter has posted comments on this change. ( https://gerrit.osmocom.org/11521 )
Change subject: add DLCX command statistics to osmo-mgw
..
Patch Set 4:
(1 comment)
> (1 comment)
(making sure my comment gets posted)
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@116
PS4, Line 116: [MGCP_DLCX_FAIL_NO_RTP] = {"dlcx:no_rtp", "no rtp stream
associated with connection."},
> I don't think this makes sense from a user perspective ... […]
I have checked this. When I get the things right, this counter would increment
when some call agent tries to delete a connection, but issues a wrong call-id.
>From my perspective it would be ok to count this one as well as an increased
>amount of such errors would point out a call id handling bug in the call agent
>so I think its useful enough to keep it.
What I would check is if the naming is consistant. Above I can see a
MGCP_MDCX_FAIL_INVALID_CALLID for mdcx. Maybe this can be coherent.
--
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
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Pau Espin Pedrol
Gerrit-Reviewer: Stefan Sperling
Gerrit-Reviewer: dexter
Gerrit-Comment-Date: Mon, 19 Nov 2018 11:39:54 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 4: (1 comment) 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@1372 PS4, Line 1372: rate_ctr_inc(&rate_ctrs->ctr[MGCP_DLCX_FAIL_NO_RTP]); > IIUC when we don't have this kind of conn, then the endpoint shouldn't exist. > […] I don't believe dexter is reading along, so he might not be aware of your question to him. I have added him as a reviewer now. -- 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 Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Reviewer: dexter Gerrit-Comment-Date: Fri, 09 Nov 2018 10:19:47 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 4: (1 comment) 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@a1510 PS4, Line 1510: > rate_ctr_group_alloc() may return NULL also for invalid naming, so I would > rather keep these. Seriously, you want me to change this _again_? Have you read the previous discussion? It seems this entire patch is getting hung up just because we won't agree on something as basic as how to check for allocation failures in osmocom. I'd rather keep it as it is now instead of bringing this question back up with Pau and Harald all over again. The code it would likely crash in talloc_set_destructor() on the next line anyway. By the way, shouldn't rate_ctr_group_alloc() allow its caller to differentiate between memory allocation failures and an invalid counter group description? -- 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 Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Fri, 09 Nov 2018 10:16:12 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
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: Code-Review-1
(4 comments)
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@a1510
PS4, Line 1510:
rate_ctr_group_alloc() may return NULL also for invalid naming, so I would
rather keep these.
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 don't think this makes sense from a user perspective ... it's more of an
internal error that some part of the endpoint isn't configured properly ...
(continuing to comment where it is used)
https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@1356
PS4, Line 1356: num_conns, ENDPOINT_NUMBER(endp));
this above change is completely orthogonal. I'd argue to keep it in a separate
patch, by general principle. Well, ok, just keep it in mind next time... (says
the guy with a recurring tradition of dropping humungous code bombs)
https://gerrit.osmocom.org/#/c/11521/4/src/libosmo-mgcp/mgcp_protocol.c@1372
PS4, Line 1372:
rate_ctr_inc(&rate_ctrs->ctr[MGCP_DLCX_FAIL_NO_RTP]);
IIUC when we don't have this kind of conn, then the endpoint shouldn't exist.
MGCP wise: after CRCX, a conn exists right away. After the DLCX on the last
conn, the endpoint is gone. So if this happens it's an internal error, more
like FAIL_NO_CONN. In practice it will never count up. Right, dexter?
--
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
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Pau Espin Pedrol
Gerrit-Reviewer: Stefan Sperling
Gerrit-Comment-Date: Thu, 08 Nov 2018 16:24:41 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 4: (1 comment) 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: > Harald: "We don't use OSMO_ASSERT() on our allocations, do we?" […] Fine, so it seems Harald has changed his mind, or I misunderstood when we talked months ago. Assertions removed in next patch set. -- 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 Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Thu, 08 Nov 2018 09:51:56 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Hello Pau Espin Pedrol, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11521 to look at the new patch set (#4). Change subject: add DLCX command statistics to osmo-mgw .. add DLCX command statistics to osmo-mgw Add a counter group for DLCX commands. The group contains counters for successful connection processing as well as various error conditions. This provides a quick overview of DLCX failures on each trunk throughout the lifetime of the osmo-mgw process. The counters are displayed by 'show mgcp stats' and 'show rate-counters' While here, rename MGCP_MDCX_FAIL_DEFERRED_BY_POLICY to MGCP_MDCX_DEFERRED_BY_POLICY; we have decided that deferred connections aren't failures, and this keeps names used by DLCX and MDCX in sync. Also remove some allocation failure checks with OSMO_ASSERT(); such checks aren't en vogue anymore. Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Depends: I80d36181600901ae2e0f321dc02b5d54ddc94139I Related: OS#2660 --- M include/osmocom/mgcp/mgcp.h M src/libosmo-mgcp/mgcp_protocol.c M src/libosmo-mgcp/mgcp_vty.c 3 files changed, 68 insertions(+), 11 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/21/11521/4 -- 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: newpatchset Gerrit-Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Gerrit-Change-Number: 11521 Gerrit-PatchSet: 4 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 3: Code-Review+1 -- 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: 3 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Tue, 06 Nov 2018 11:13:01 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 1: (1 comment) 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: OSMO_ASSERT(trunk->mgcp_dlcx_ctr_group); > Can you please be more specific? How strongly do you care about the > OSMO_ASSERT? […] Harald: "We don't use OSMO_ASSERT() on our allocations, do we?" Pau: "That's first notice I get regarding we use OSMO_ASSERT during allocations, I would say we usually don't." I think we are both on the same opinion here? -- 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: 1 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Tue, 06 Nov 2018 11:11:25 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Hello Pau Espin Pedrol, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11521 to look at the new patch set (#3). Change subject: add DLCX command statistics to osmo-mgw .. add DLCX command statistics to osmo-mgw Add a counter group for DLCX commands. The group contains counters for successful connection processing as well as various error conditions. This provides a quick overview of DLCX failures on each trunk throughout the lifetime of the osmo-mgw process. The counters are displayed by 'show mgcp stats' and 'show rate-counters' While here, rename MGCP_MDCX_FAIL_DEFERRED_BY_POLICY to MGCP_MDCX_DEFERRED_BY_POLICY; we have decided that deferred connections aren't failures, and this keeps names used by DLCX and MDCX in sync. Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Depends: I80d36181600901ae2e0f321dc02b5d54ddc94139I Related: OS#2660 --- M include/osmocom/mgcp/mgcp.h M src/libosmo-mgcp/mgcp_protocol.c M src/libosmo-mgcp/mgcp_vty.c 3 files changed, 69 insertions(+), 8 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/21/11521/3 -- 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: newpatchset Gerrit-Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Gerrit-Change-Number: 11521 Gerrit-PatchSet: 3 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 2: (1 comment) 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: * (called once at startup by main function) */ > That's first notice I get regarding we use OSMO_ASSERT during allocations, I > would say we usually do […] Can you please be more specific? How strongly do you care about the OSMO_ASSERT? I am getting different opinions from you and Harald. I have already indicated that I will do whatever both of you want, but you must make non-contradictory suggestions. -- 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: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Tue, 06 Nov 2018 11:00:39 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 2: Code-Review+2 -- 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: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-Comment-Date: Wed, 31 Oct 2018 22:14:11 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 1: (2 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@1347 PS1, Line 1347: rate_ctr_inc(&rate_ctrs->ctr[MGCP_DLCX_FAIL_REJECTED_BY_POLICY]); > I'd prefer consistency over making exceptions. […] Well, deferring in this case is more a success than a failure, that's what I'm trying to tell you. https://gerrit.osmocom.org/#/c/11521/1/src/libosmo-mgcp/mgcp_protocol.c@1573 PS1, Line 1573: OSMO_ASSERT(trunk->mgcp_dlcx_ctr_group); > If it were up to me, all allocation failures in osmocom projects would be > handled like regular error […] That's first notice I get regarding we use OSMO_ASSERT during allocations, I would say we usually don't. -- 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: 1 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-CC: Harald Welte Gerrit-Comment-Date: Wed, 31 Oct 2018 12:17:15 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 2: (1 comment) 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@1347 PS1, Line 1347: } > I'd say that's not a failure, but an app decision to handle it its own way, > so better remove FAIL fr […] I'd prefer consistency over making exceptions. I think it is fine to treat this and the case below like any other non-success cases. -- 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: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-CC: Harald Welte Gerrit-Comment-Date: Wed, 31 Oct 2018 12:06:26 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 2: (1 comment) 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: * (called once at startup by main function) */ > It doesn't hurt either. […] If it were up to me, all allocation failures in osmocom projects would be handled like regular errors, i.e. propagated back to callers. But our code was written with a different style. Back when I first asked about this, I remember Harald specifically telling me that osmocom projects check allocations with OSMO_ASSERT(), so that's what I've been doing. It's fine if you guys change your minds, but consistency will be appreciated nevertheless :) -- 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: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Reviewer: Stefan Sperling Gerrit-CC: Harald Welte Gerrit-Comment-Date: Wed, 31 Oct 2018 12:02:08 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Hello Pau Espin Pedrol, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/11521 to look at the new patch set (#2). Change subject: add DLCX command statistics to osmo-mgw .. add DLCX command statistics to osmo-mgw Add a counter group for DLCX commands. The group contains counters for successful connection processing as well as various error conditions. This provides a quick overview of DLCX failures on each trunk throughout the lifetime of the osmo-mgw process. The counters are displayed by 'show mgcp stats' and 'show rate-counters' Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Depends: I80d36181600901ae2e0f321dc02b5d54ddc94139I Related: OS#2660 --- M include/osmocom/mgcp/mgcp.h M src/libosmo-mgcp/mgcp_protocol.c M src/libosmo-mgcp/mgcp_vty.c 3 files changed, 66 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/21/11521/2 -- 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: newpatchset Gerrit-Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b Gerrit-Change-Number: 11521 Gerrit-PatchSet: 2 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-CC: Harald Welte
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 1: (1 comment) 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: OSMO_ASSERT(trunk->mgcp_dlcx_ctr_group); > I think we can drop this kind of check, unless rate_ctr_group_alloc does a > lot more than just clalin […] It doesn't hurt either. We don't use OSMO_ASSERT() on our allocations, do we? -- 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: 1 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-CC: Harald Welte Gerrit-Comment-Date: Wed, 31 Oct 2018 08:15:27 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/11521 ) Change subject: add DLCX command statistics to osmo-mgw .. Patch Set 1: Code-Review-1 (3 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@1347 PS1, Line 1347: rate_ctr_inc(&rate_ctrs->ctr[MGCP_DLCX_FAIL_REJECTED_BY_POLICY]); I'd say that's not a failure, but an app decision to handle it its own way, so better remove FAIL from it. https://gerrit.osmocom.org/#/c/11521/1/src/libosmo-mgcp/mgcp_protocol.c@1354 PS1, Line 1354: rate_ctr_inc(&rate_ctrs->ctr[MGCP_DLCX_FAIL_DEFERRED_BY_POLICY]); Same here. https://gerrit.osmocom.org/#/c/11521/1/src/libosmo-mgcp/mgcp_protocol.c@1573 PS1, Line 1573: OSMO_ASSERT(trunk->mgcp_dlcx_ctr_group); I think we can drop this kind of check, unless rate_ctr_group_alloc does a lot more than just claling talloc/malloc. -- 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: 1 Gerrit-Owner: Stefan Sperling Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Tue, 30 Oct 2018 14:47:23 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-mgw[master]: add DLCX command statistics to osmo-mgw
Stefan Sperling has uploaded this change for review. (
https://gerrit.osmocom.org/11521
Change subject: add DLCX command statistics to osmo-mgw
..
add DLCX command statistics to osmo-mgw
Add a counter group for DLCX commands. The group contains counters for
successful connection processing as well as various error conditions.
This provides a quick overview of DLCX failures on each trunk throughout
the lifetime of the osmo-mgw process.
The counters are displayed by 'show mgcp stats' and 'show rate-counters'
Change-Id: Ie0dde2faf02fd68a69f986973d39b1bea367039b
Depends: I80d36181600901ae2e0f321dc02b5d54ddc94139I
Related: OS#2660
---
M include/osmocom/mgcp/mgcp.h
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_vty.c
3 files changed, 67 insertions(+), 6 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/21/11521/1
diff --git a/include/osmocom/mgcp/mgcp.h b/include/osmocom/mgcp/mgcp.h
index 4a307cd..d08c3ef 100644
--- a/include/osmocom/mgcp/mgcp.h
+++ b/include/osmocom/mgcp/mgcp.h
@@ -155,6 +155,18 @@
MGCP_MDCX_FAIL_DEFERRED_BY_POLICY
};
+/* Global MCGP DLCX related rate counters */
+enum {
+ MGCP_DLCX_SUCCESS,
+ MGCP_DLCX_FAIL_WILDCARD,
+ MGCP_DLCX_FAIL_NO_CONN,
+ MGCP_DLCX_FAIL_INVALID_CALLID,
+ MGCP_DLCX_FAIL_INVALID_CONNID,
+ MGCP_DLCX_FAIL_UNHANDLED_PARAM,
+ MGCP_DLCX_FAIL_REJECTED_BY_POLICY,
+ MGCP_DLCX_FAIL_DEFERRED_BY_POLICY,
+ MGCP_DLCX_FAIL_NO_RTP
+};
struct mgcp_trunk_config {
struct llist_head entry;
@@ -198,6 +210,8 @@
struct rate_ctr_group *mgcp_crcx_ctr_group;
/* Rate counter group which contains stats for processed MDCX commands.
*/
struct rate_ctr_group *mgcp_mdcx_ctr_group;
+ /* Rate counter group which contains stats for processed DLCX commands.
*/
+ struct rate_ctr_group *mgcp_dlcx_ctr_group;
/* Rate counter group which aggregates stats of individual RTP
connections. */
struct rate_ctr_group *all_rtp_conn_stats;
};
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index 767faa9..b9469ec 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -73,7 +73,7 @@
const static struct rate_ctr_group_desc mgcp_crcx_ctr_group_desc = {
.group_name_prefix = "crcx",
- .group_description = "crxc statistics",
+ .group_description = "crcx statistics",
.class_id = OSMO_STATS_CLASS_GLOBAL,
.num_ctr = ARRAY_SIZE(mgcp_crcx_ctr_desc),
.ctr_desc = mgcp_crcx_ctr_desc
@@ -104,6 +104,26 @@
.ctr_desc = mgcp_mdcx_ctr_desc
};
+static const struct rate_ctr_desc mgcp_dlcx_ctr_desc[] = {
+ [MGCP_DLCX_SUCCESS] = {"dlcx:success", "DLCX command processed
successfully."},
+ [MGCP_DLCX_FAIL_WILDCARD] = {"dlcx:wildcard", "wildcard names in DLCX
commands are unsupported."},
+ [MGCP_DLCX_FAIL_NO_CONN] = {"dlcx:no_conn", "endpoint specified in DLCX
command has no active connections."},
+ [MGCP_DLCX_FAIL_INVALID_CALLID] = {"dlcx:callid", "invalid CallId
specified in DLCX command."},
+ [MGCP_DLCX_FAIL_INVALID_CONNID] = {"dlcx:connid", "invalid connection
ID specified in DLCX command."},
+ [MGCP_DLCX_FAIL_UNHANDLED_PARAM] = {"dlcx:unhandled_param", "unhandled
parameter in DLCX command."},
+ [MGCP_DLCX_FAIL_REJECTED_BY_POLICY] = {"dlcx:rejected", "connection
deletion rejected by policy."},
+ [MGCP_DLCX_FAIL_DEFERRED_BY_POLICY] = {"dlcx:deferred", "connection
deletion deferred by policy."},
+ [MGCP_DLCX_FAIL_NO_RTP] = {"dlcx:no_rtp", "no rtp stream associated
with connection."},
+};
+
+const static struct rate_ctr_group_desc mgcp_dlcx_ctr_group_desc = {
+ .group_name_prefix = "dlcx",
+ .group_description = "dlcx statistics",
+ .class_id = OSMO_STATS_CLASS_GLOBAL,
+ .num_ctr = ARRAY_SIZE(mgcp_dlcx_ctr_desc),
+ .ctr_desc = mgcp_dlcx_ctr_desc
+};
+
/* Must be kept in sync with rate_ctr_desc in mgcp_conn.c */
static const struct rate_ctr_desc all_rtp_conn_rate_ctr_desc[] = {
[IN_STREAM_ERR_TSTMP_CTR] = {"all_rtp:err_tstmp_in", "Total inbound
rtp-stream timestamp errors."},
@@ -1251,7 +1271,9 @@
/* DLCX command handler, processes the received command */
static struct msgb *handle_delete_con(struct mgcp_parse_data *p)
{
+ struct mgcp_trunk_config *tcfg = p->endp->tcfg;
struct mgcp_endpoint *endp = p->endp;
+ struct rate_ctr_group *rate_ctrs = tcfg->mgcp_dlcx_ctr_group;
int error_code = 400;
int silent = 0;
char *line;
@@ -1268,6 +1290,7 @@
LOGP(DLMGCP, LOGL_ERROR,
"DLCX: endpoint:0x%x wildcarded endpoint names not
supported.\n",
ENDPOINT_NUMBER(endp));
+ rate_ctr_inc(&rate_ctrs->ctr[MGCP_DLCX_FAIL_WILDCARD]);
return create_err_response(endp, 507, "DL
