<p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/gprs_bssgp_rim.c">File src/gprs_bssgp_rim.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/osmo-pcu/+/22385/9/src/gprs_bssgp_rim.c@142">Patch Set #9, Line 142:</a> <code style="font-family:monospace,monospace">                   "Rx RAN-INFO with an app error! cause: %s\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think all of those log messages about incoming RIM should always print a stringified verson of the source address, as otherwise it will be pretty difficult to debug in any real workld network, where tons of other cells will be sending us RIM requests</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c">File src/nacc_fsm.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/osmo-pcu/+/22385/9/src/nacc_fsm.c@55">Patch Set #9, Line 55:</a> <code style="font-family:monospace,monospace">static struct msgb *create_packet_neighbour_cell_data(struct nacc_fsm_ctx *ctx, struct gprs_rlcmac_tbf *tbf)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const for tbf? or is it written to?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c@139">Patch Set #9, Line 139:</a> <code style="font-family:monospace,monospace">static struct msgb *create_packet_cell_chg_continue(struct nacc_fsm_ctx *ctx, struct gprs_rlcmac_tbf *tbf)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same here: const?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c@182">Patch Set #9, Line 182:</a> <code style="font-family:monospace,monospace">static int fill_rim_ran_info_req(struct nacc_fsm_ctx *ctx, struct bssgp_ran_information_pdu *pdu)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const for the 'ctx'? usually output argument as first argument (unless pcu uses different conventions)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.h">File src/neigh_cache.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/osmo-pcu/+/22385/9/src/neigh_cache.h@49">Patch Set #9, Line 49:</a> <code style="font-family:monospace,monospace">    struct llist_head list; /* to be included in neigh_cache->list */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">could possibly go for hashtable right away, now that we have it in libosmocore?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.h@72">Patch Set #9, Line 72:</a> <code style="font-family:monospace,monospace">       struct llist_head list; /* list of si_cache_entry items */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">likewise, could use hashtable.h</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c">File src/neigh_cache.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/osmo-pcu/+/22385/9/src/neigh_cache.c@33">Patch Set #9, Line 33:</a> <code style="font-family:monospace,monospace">static void neigh_cache_cleanup_cb(void *data) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we normally have '{' on a separate line in C...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@53">Patch Set #9, Line 53:</a> <code style="font-family:monospace,monospace">static void neigh_cache_schedule_cleanup(struct neigh_cache *cache) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we normally have '{' on a separate line in C...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@93">Patch Set #9, Line 93:</a> <code style="font-family:monospace,monospace">               it = talloc_zero(cache, struct neigh_cache_entry);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no OSMO_ASSERT() or NULL check after allocation</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@149">Patch Set #9, Line 149:</a> <code style="font-family:monospace,monospace">static void si_cache_cleanup_cb(void *data) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see above</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@169">Patch Set #9, Line 169:</a> <code style="font-family:monospace,monospace">static void si_cache_schedule_cleanup(struct si_cache *cache) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">style, see above</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@193">Patch Set #9, Line 193:</a> <code style="font-family:monospace,monospace">  struct si_cache *cache = talloc_zero(ctx, struct si_cache);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no ASSERT or NULL check after allocation</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@208">Patch Set #9, Line 208:</a> <code style="font-family:monospace,monospace">             it = talloc_zero(cache, struct si_cache_entry);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no ASSERT or NULL check after allocation</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385">change 22385</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/c/osmo-pcu/+/22385"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id35f40d05f3e081f32fddbf1fa34cb338db452ca </div>
<div style="display:none"> Gerrit-Change-Number: 22385 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 27 Jan 2021 10:59:22 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>