Attention is currently required from: daniel, lynxis lazus.

Patch set 5:Code-Review -1

View Change

11 comments:

• File include/osmocom/sgsn/gprs_routing_area.h:

  • Patch Set #5, Line 14: struct osmo_routing_area_id ra;

    rai? raid?

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

  • Patch Set #5, 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:

  • Patch Set #5, 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:

  • Patch Set #5, Line 15: /* FIXME: move to sgsn global context */

    why not doing this already? sounds easy?

  • Patch Set #5, Line 27: struct sgsn_ra_cell *cell, *cell2;

    in general free functions allow null ptr.
    if (!ra) return NULL;

  • Patch Set #5, Line 29: if (!llist_empty(&ra->cells)) {

    this one is not needed right? the for_each below will simply skip.

  • Patch Set #5, Line 43: OSMO_ASSERT(cell);

    in general free functions allow null ptr.
    if (!cell) return NULL;

  • Patch Set #5, Line 45: if (!drop_empty_ra) {

    all this code below is a bit chaotic, I encourage you to revisit it.

  • Patch Set #5, 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()

  • Patch Set #5, 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.

  • Patch Set #5, Line 281: }

    All this function is also a bit cumbersome, I encourage doing a second thought too.

To view, visit change 37864. To unsubscribe, or for help writing mail filters, visit settings.