Attention is currently required from: neels.

View Change

20 comments:

• File include/osmocom/hnbgw/context_map.h:

  • Patch Set #1, Line 56: * and each Response gets one ps_rab_ass FSM. Each such message can contain any number and any combination of

    you mean each req+response pair has a ps_rab_ass? or one for each?

• File include/osmocom/hnbgw/hnbgw.h:

  • Patch Set #1, Line 193: static inline bool hnb_gw_is_gtp_mapping_enabled(const struct hnb_gw *gw)

    can you document this function? what does gtp_mapping mean exactly?

• File include/osmocom/hnbgw/ps_rab_fsm.h:

  • Patch Set #1, Line 8: struct addr_teid {

    Maybe add a one line documenttion. Is this for GTP? gtp_addr_teid?

  • Patch Set #1, Line 14: /* Two of this make up a GTP mapping, one for the Access (HNB) side, one for the Core (CN) side.

    what does "this" mean here?

  • Patch Set #1, Line 24: struct half_gtp_map {

    gtp_map_leg? to use some term more "usual".

  • Patch Set #1, Line 36: /* Whether the RANAP message this RAB's remote address was obtained from encoded the address in x213_nsap */

    "from encoded the address" looks wrongly written?

  • Patch Set #1, Line 41: struct osmo_fsm_inst *fi;

    document the type of fsm? because I see 2 more "fi" below.

  • Patch Set #1, Line 54: /* Backpointer to the ps_rab_ass_fsm for the RAB Assignment Request from Core that created this RAB. */

    (I wonder why do we need 2 different FSMs here, looks like the same followup procedure to me, req + resp).

  • Patch Set #1, Line 79: bool ps_rab_is_established(struct ps_rab *rab);

    constify param

• File src/osmo-hnbgw/hnbgw.c:

  • Patch Set #1, Line 230: LOGP(DHNBAP, LOGL_INFO, "deallocating UE context: id 0x%x, imsi %s, tmsi 0x%x\n",

    this can probably go on a different commit.

  • Patch Set #1, Line 354: LOGP(DMAIN, LOGL_INFO, "Deallocating hnb_context\n");

    this can probably go on a different commit.

• File src/osmo-hnbgw/hnbgw_cn.c:

  • Patch Set #1, Line 373: if (!map->is_ps) {

    not super important, but probably makes it easier to read if you do "if (map->is_ps)" now, and move the existing code to the "else" condition.

  • Patch Set #1, Line 396: rc = ranap_ran_rx_co_decode(map, message, msgb_l2(oph->msg), msgb_l2len(oph->msg));

    You are actually duplicating these 2 lines now. They can be moved outside the if/else condition. The "if (rc == 0)" and ranap_ran_rx_co_free(message); can also be in the single path.

• File src/osmo-hnbgw/hnbgw_pfcp.c:

  • Patch Set #1, Line 77: if (!hnb_gw_is_gtp_mapping_enabled(hnb_gw)) {

    I'd move this to the main function, to avoid calling hnbgw_pfcp_init(). I think for readers will be easier to understand the whole thing is disabled by this when looking at the main file.

• File src/osmo-hnbgw/hnbgw_rua.c:

  • Patch Set #1, Line 284: if (!map->is_ps) {

    same, probably easier to read with "if (map->is_ps)"

• File src/osmo-hnbgw/hnbgw_vty.c:

  • Patch Set #1, Line 474: if (g_hnb_gw->config.pfcp.local_addr)

    I think we usually print first local addresses, then remote addresses.

• File src/osmo-hnbgw/ps_rab_ass_fsm.c:

  • Patch Set #1, Line 490: /* See whether all RABs are done establishing, and replace RTP info in the RAB Assignment Response message, so that we

    s/RTP/GTP/

  • Patch Set #1, Line 625: void hnbgw_gtpmap_release(struct hnbgw_context_map *map)

    I'm reading "gsmtap" all the time ;)

• File src/osmo-hnbgw/ps_rab_fsm.c:

  • Patch Set #1, Line 786: .name = "PS_RAB_ST_WAIT_PFCP_DEL_RESP",

    WAIT_PFCP_DEL_RESP

  • Patch Set #1, Line 797: .name = "PS_RAB_ST_WAIT_USE_COUNT",

    WAIT_USE_COUNT

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