<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/8086">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/8086/1/include/osmocom/mgcp/mgcp_internal.h">File include/osmocom/mgcp/mgcp_internal.h:</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/8086/1/include/osmocom/mgcp/mgcp_internal.h@31">Patch Set #1, Line 31:</a> <code style="font-family:monospace,monospace">#include <osmocom/core/rate_ctr.h></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(could instead just define the opaque structs here, since this header is only using pointers)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/8086/1/include/osmocom/mgcp/mgcp_internal.h@49">Patch Set #1, Line 49:</a> <code style="font-family:monospace,monospace"> struct rate_ctr *err_ts_ctr;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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. </p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> osmo-bsc/src/libbsc/abis_rsl.c:98: rate_ctr_inc(&bts->bts_ctrs->ctr[BTS_CTR_CODEC_AMR_H]);</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_conn.c">File src/libosmo-mgcp/mgcp_conn.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/8086/1/src/libosmo-mgcp/mgcp_conn.c@32">Patch Set #1, Line 32:</a> <code style="font-family:monospace,monospace">const static struct rate_ctr_desc rate_ctr_desc[] = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_conn.c@34">Patch Set #1, Line 34:</a> <code style="font-family:monospace,monospace"> .name = "in_stream_err_ts_ctr",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see you essentially took over the old naming; but here is some naming nitpicking, if you care enough:</p><ul><li>we know it is a counter from the API now, no need for "_ctr"</li><li>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"?</li><li>"ts" commonly means "timeslot" in osmo-bsc, I would prefer not to re-use that abbreviation. spell out "timeslot_err" completely?</li></ul></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/8086/1/src/libosmo-mgcp/mgcp_network.c">File src/libosmo-mgcp/mgcp_network.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/8086/1/src/libosmo-mgcp/mgcp_network.c@508">Patch Set #1, Line 508:</a> <code style="font-family:monospace,monospace"> /* FIXME: Move this initialization to mgcp.conn.c */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">underscore? and what he said below</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/8086/1/tests/mgcp/mgcp_test.c">File tests/mgcp/mgcp_test.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/8086/1/tests/mgcp/mgcp_test.c@1150">Patch Set #1, Line 1150:</a> <code style="font-family:monospace,monospace"> state.out_stream.err_ts_ctr = &test_ctr_out;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(re using indexes, could instead init the state.rate_ctr_group here)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/8086">change 8086</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/8086"/><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: I9fbd65bf2f4d1e015a05996db4c1f7ff20be2c95 </div>
<div style="display:none"> Gerrit-Change-Number: 8086 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 14 May 2018 15:10:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>