Change in osmo-iuh[master]: mgw_fsm: add MGW support to osmo-hnbgw

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 -- To view, visit https://gerrit.osmocom.org/c/osmo-iuh/+/26725 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-iuh Gerrit-Branch: master Gerrit-Change-Id: Ib9b62e0145184b91c56ce5d8870760bfa49cc5a4 Gerrit-Change-Number: 26725 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 04 Jan 2022 13:34:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment