Change in osmo-mgw[master]: stats: use libosmocore rate counter for in/out_stream.err_ts_counter

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/8086 )

Change subject: stats: use libosmocore rate counter for in/out_stream.err_ts_counter
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

https://gerrit.osmocom.org/#/c/8086/1/include/osmocom/mgcp/mgcp_internal.h
File include/osmocom/mgcp/mgcp_internal.h:

https://gerrit.osmocom.org/#/c/8086/1/include/osmocom/mgcp/mgcp_internal.h@31
PS1, Line 31: #include <osmocom/core/rate_ctr.h>
(could instead just define the opaque structs here, since this header is only using pointers)


https://gerrit.osmocom.org/#/c/8086/1/include/osmocom/mgcp/mgcp_internal.h@49
PS1, Line 49: 	struct rate_ctr *err_ts_ctr;
is it worthwhile to keep a separate pointer? It brings trouble when a rate_ctr_group is ever freed: the code needs to take special care to also invalidate this pointer. Typically we just index into the rate_ctr_group->ctr[], see e.g. 

  osmo-bsc/src/libbsc/abis_rsl.c:98:			rate_ctr_inc(&bts->bts_ctrs->ctr[BTS_CTR_CODEC_AMR_H]);


https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_conn.c
File src/libosmo-mgcp/mgcp_conn.c:

https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_conn.c@32
PS1, Line 32: const static struct rate_ctr_desc rate_ctr_desc[] = {
typically we define an enum for index values and later on use the names, which avoids magic numbers and index mistakes, and greatly simplifies changing this array in the future. See for example osmo-bsc/include/osmocom/bsc/gsm_data.h bts_ctr_description


https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_conn.c@34
PS1, Line 34: 		.name = "in_stream_err_ts_ctr",
I see you essentially took over the old naming; but here is some naming nitpicking, if you care enough:

* we know it is a counter from the API now, no need for "_ctr"
* does it make sense to see "err_ts" as the common part, and "in" and "out" are aspects of it? I mean "err_ts_in", "err_ts_out"?
* "ts" commonly means "timeslot" in osmo-bsc, I would prefer not to re-use that abbreviation. spell out "timeslot_err" completely?


https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_network.c
File src/libosmo-mgcp/mgcp_network.c:

https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_network.c@508
PS1, Line 508: 		/* FIXME: Move this initialization to mgcp.conn.c */
underscore? and what he said below


https://gerrit.osmocom.org/#/c/8086/1/tests/mgcp/mgcp_test.c
File tests/mgcp/mgcp_test.c:

https://gerrit.osmocom.org/#/c/8086/1/tests/mgcp/mgcp_test.c@1150
PS1, Line 1150: 	state.out_stream.err_ts_ctr = &test_ctr_out;
(re using indexes, could instead init the state.rate_ctr_group here)



-- 
To view, visit https://gerrit.osmocom.org/8086
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: I9fbd65bf2f4d1e015a05996db4c1f7ff20be2c95
Gerrit-Change-Number: 8086
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Mon, 14 May 2018 15:10:04 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180514/b782ec36/attachment.htm>