pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/22385 )
Change subject: Introduce NACC support ...................................................................... Patch Set 9: (7 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 […] Ack 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? I'll check if any function requires it to be non-null, it could be. 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 convention […] const: ack. 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. 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? 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. 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... Ack 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... Ack 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 Usual rant about whether malloc can fail or not :P I'll add it. -- 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 11:34:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <[email protected]> Gerrit-MessageType: comment
