pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-iuh/+/26725 )

Change subject: mgw_fsm: add MGW support to osmo-hnbgw ......................................................................

Patch Set 1:

(15 comments)

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_cn.c File src/hnbgw_cn.c:

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_cn.c@367 PS1, Line 367: * its own copy of the message contents, so it is safe to free oph->msg() in sccp_sap_up() below */ when you say "sccp_sap_up() below", you mean the caller of handle_cn_data_ind freed the msgb?

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_cn.c@369 PS1, Line 369: } else if (msg_len > 2 && msg_data[1] == RANAP_ProcedureCode_id_Iu_Release && !map->is_ps && map->mgw_fi) { Probably several levels here look far more structured and extensible: if (msg_len > 2) { switch (msg_data[1]): case RANAP_ProcedureCode_id_RAB_Assignment: if (!map->is_ps) ... break; }

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_rua.c File src/hnbgw_rua.c:

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_rua.c@232 PS1, Line 232: /* TODO: Check for map and if this is a RAB Assignment response what about this?

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_rua.c@278 PS1, Line 278: if (map && !release_context_map && msg_len > 2 && msg_data[1] == RANAP_ProcedureCode_id_RAB_Assignment same, probably splitting into several levels makes it more easy to avoid future issues when extending this.

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_vty.c File src/hnbgw_vty.c:

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/hnbgw_vty.c@409 PS1, Line 409: mgcp_client_config_write(vty, " "); are you sure this prefix is correct? aren't you missing an extra space? There should probably be two of them.

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c File src/mgw_fsm.c:

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@69 PS1, Line 69: MGW_ST_CRCX_HNB, WAIT_CRCX?

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@71 PS1, Line 71: MGW_ST_MDCX_HNB, WAIT_MDCX?

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@86 PS1, Line 86: uint8_t rab_ass_req[IUH_MSGB_SIZE]; Use "struct *msgb" here and mgcp_copy()?

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@87 PS1, Line 87: unsigned int rab_ass_req_len; You don't need this field if you copy the msgb.

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@190 PS1, Line 190: unneeded space

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@243 PS1, Line 243: osmo_tdef_fsm_inst_state_chg(fi, MGW_ST_CRCX_MSC, mgw_fsm_timeouts, mgw_fsm_T_defs, -1); You can probably use your own define macro here to only pass fi and new state, see other FSMs

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@414 PS1, Line 414: .in_event_mask = 0 I find this "0 |" way quite strange and confusing. I think we usually do: .in_event_mask = S(MGW_EV_MGCP_OK) | S(MGW_EV_MGCP_FAIL),

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@485 PS1, Line 485: .allstate_event_mask = 0 | S(MGW_EV_MGCP_TERM) Please clean up this line(s).

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/mgw_fsm.c@500 PS1, Line 500: struct osmo_fsm_inst *osmo_fsm_inst; You can probably drop this variable.

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/tdefs.c File src/tdefs.c:

https://gerrit.osmocom.org/c/osmo-iuh/+/26725/1/src/tdefs.c@20 PS1, Line 20: {.T = -1001,.default_val = 5,.desc = "Timeout for HNB side call-leg (to-HNB) creation" }, missing space after comma in all lines