Attention is currently required from: pespin.
11 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?
one each. tried to think of an ascii art way to draw the relations because it is strugglesome to explain in words ...
first i tried to have one FSM for req + resp + PFCP for all RABs, then gradually noticed that each RAB needs a separate PFCP FSM, and then that i also need to have separate FSMs for RANAP req and resp.
File include/osmocom/hnbgw/ps_rab_fsm.h:
Patch Set #1, Line 24: struct half_gtp_map {
gtp_map_leg? to use some term more "usual".
gtp_tunnel would be accurate, except that the FAR (Forwarding Action Rule) is closely related to the *other* GTP tunnel. I'll expand the comment.
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?
it is accurate but i'll rephrase to less ambiguous grammar
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 + […]
commenting in code...
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.
missed this one, i think i needed it for debugging, could just drop it
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 […]
we (i?) usually list CS above PS ... so the is_ps maybe should have been named is_cs instead; i agree that it is slightly annoying, but i've made peace with that over the years
(this line is unchanged from before this patch despite gerrit rendering on green background, I'm only adding an 'else')
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. […]
i think i even considered that when writing the patch, but left it like this to show that the CS code is completely unchanged... but true.
i'll remove the code dup in a separate patch
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(). […]
that's a matter of taste ... personally i like it very much when things are completely contained in a single file, with the parts appearing in main() as minimal as possible.
but i see the LOGP above seems at the wrong place
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)"
idk i like CS above PS better, also before this patch there was "!is_ps" above
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.
same order here as where the cmds are added to vty, but ok whatever
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/
lol thx
To view, visit change 28816. To unsubscribe, or for help writing mail filters, visit settings.