Change in osmo-pcu[master]: Introduce NACC support

[pespin]

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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Wed, 27 Jan 2021 11:34:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210127/c3a6e5eb/attachment.htm>