Attention is currently required from: daniel, lynxis lazus.

pespin has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/37864?usp=email )

Change subject: add routing areas ......................................................................

Patch Set 5: Code-Review-1

(11 comments)

File include/osmocom/sgsn/gprs_routing_area.h:

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/b7c5393d_4136b025?usp... : PS5, Line 14: struct osmo_routing_area_id ra; rai? raid?

Otherwise we end up with ra->ra which is confusing.

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/61ff94b8_d2ae8a13?usp... : PS5, Line 50: void sgsn_ra_cell_free(struct sgsn_ra_cell *cell, bool drop_empty_ra); disturbing that it has a free function but not an alloc function. This probably needs rethinking.

File src/sgsn/gprs_bssgp.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/137ab8d2_89da0d68?usp... : PS5, Line 40: int bssgp_nm_bvc_reset_ind(struct osmo_bssgp_prim *bp) this can be static afaict?

File src/sgsn/gprs_routing_area.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/2e8beec6_23b4d5cb?usp... : PS5, Line 15: /* FIXME: move to sgsn global context */ why not doing this already? sounds easy?

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/c93acb41_5c07420c?usp... : PS5, Line 27: struct sgsn_ra_cell *cell, *cell2; in general free functions allow null ptr. if (!ra) return NULL;

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/ae30db36_fe4b93f9?usp... : PS5, Line 29: if (!llist_empty(&ra->cells)) { this one is not needed right? the for_each below will simply skip.

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/1be3f837_b953cb1f?usp... : PS5, Line 43: OSMO_ASSERT(cell); in general free functions allow null ptr. if (!cell) return NULL;

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/e324e74e_8c23264c?usp... : PS5, Line 45: if (!drop_empty_ra) { all this code below is a bit chaotic, I encourage you to revisit it.

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/7c0eb2e6_385ccb35?usp... : PS5, Line 91: struct sgsn_ra *sgsn_ra_get_ra(const struct osmo_routing_area_id *rai) sgsn_get_ra_by_rai() or sgsn_get_ra_by_raid()

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/5b2fe178_0f062e38?usp... : PS5, Line 246: } else if (cell && cell->ran_type != RA_TYPE_GERAN_Gb) { this if before this one has an early return, so this "} else" can be removed, easing code read.

https://gerrit.osmocom.org/c/osmo-sgsn/+/37864/comment/a0576c41_12d135e0?usp... : PS5, Line 281: } All this function is also a bit cumbersome, I encourage doing a second thought too.