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=email : 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=email : 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=email : 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=email : 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=email : 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=email : 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=email : 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=email : 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=email : 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=email : 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=email : PS5, Line 281: } All this function is also a bit cumbersome, I encourage doing a second thought too. -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/37864?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I2474b19a7471a1dea3c863ddf8372b16180211aa Gerrit-Change-Number: 37864 Gerrit-PatchSet: 5 Gerrit-Owner: lynxis lazus <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Attention: lynxis lazus <[email protected]> Gerrit-Comment-Date: Tue, 20 Aug 2024 19:14:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes
