<p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22385">View Change</a></p><p>7 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think all of those log messages about incoming RIM should always print a stringified verson of the […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const for tbf? or is it written to?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'll check if any function requires it to be non-null, it could be.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const for the 'ctx'? usually output argument as first argument (unless pcu uses different convention […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">const: ack.<br>order: it's arguable, I was keeping the "ctx" as first param as in object oriented, but I don't have a strong opinion here given that's a static helper.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">could possibly go for hashtable right away, now that we have it in libosmocore?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would then need to hash quite complex keys, I'll have a look but I think I'll leave it as it is. I'm not saying I'm not changing it, but I have plenty of things to do still for this task so I'm leaving this for the end.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we normally have '{' on a separate line in C...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we normally have '{' on a separate line in C...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no OSMO_ASSERT() or NULL check after allocation</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Usual rant about whether malloc can fail or not :P I'll add it.</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 11:34:40 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>