laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/22385 )
Change subject: Introduce NACC support ...................................................................... Patch Set 9: (13 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/gprs_bssgp_rim.c File src/gprs_bssgp_rim.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/gprs_bssgp_rim.c@142 PS9, Line 142: "Rx RAN-INFO with an app error! cause: %s\n", 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 https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c File src/nacc_fsm.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c@55 PS9, Line 55: static struct msgb *create_packet_neighbour_cell_data(struct nacc_fsm_ctx *ctx, struct gprs_rlcmac_tbf *tbf) const for tbf? or is it written to? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c@139 PS9, Line 139: static struct msgb *create_packet_cell_chg_continue(struct nacc_fsm_ctx *ctx, struct gprs_rlcmac_tbf *tbf) same here: const? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/nacc_fsm.c@182 PS9, Line 182: static int fill_rim_ran_info_req(struct nacc_fsm_ctx *ctx, struct bssgp_ran_information_pdu *pdu) const for the 'ctx'? usually output argument as first argument (unless pcu uses different conventions) https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.h File src/neigh_cache.h: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.h@49 PS9, Line 49: struct llist_head list; /* to be included in neigh_cache->list */ could possibly go for hashtable right away, now that we have it in libosmocore? https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.h@72 PS9, Line 72: struct llist_head list; /* list of si_cache_entry items */ likewise, could use hashtable.h https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c File src/neigh_cache.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@33 PS9, Line 33: static void neigh_cache_cleanup_cb(void *data) { we normally have '{' on a separate line in C... https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@53 PS9, Line 53: static void neigh_cache_schedule_cleanup(struct neigh_cache *cache) { we normally have '{' on a separate line in C... https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@93 PS9, Line 93: it = talloc_zero(cache, struct neigh_cache_entry); no OSMO_ASSERT() or NULL check after allocation https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@149 PS9, Line 149: static void si_cache_cleanup_cb(void *data) { see above https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@169 PS9, Line 169: static void si_cache_schedule_cleanup(struct si_cache *cache) { style, see above https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@193 PS9, Line 193: struct si_cache *cache = talloc_zero(ctx, struct si_cache); no ASSERT or NULL check after allocation https://gerrit.osmocom.org/c/osmo-pcu/+/22385/9/src/neigh_cache.c@208 PS9, Line 208: it = talloc_zero(cache, struct si_cache_entry); no ASSERT or NULL check after allocation -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/22385 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id35f40d05f3e081f32fddbf1fa34cb338db452ca Gerrit-Change-Number: 22385 Gerrit-PatchSet: 9 Gerrit-Owner: pespin <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <[email protected]> Gerrit-Reviewer: fixeria <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Comment-Date: Wed, 27 Jan 2021 10:59:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
