I will now update this patch. The comments might vanish due to the new repo?

View Change

24 comments:

• File configure.ac:

  • Patch Set #1, Line 52: PKG_CHECK_MODULES(LIBOSMOMGCPCLIENT, libosmo-mgcp-client >= 1.9.0)

    makes sense.

    Done

• File configure.ac:

  • Patch Set #2, Line 52: PKG_CHECK_MODULES(LIBOSMOMGCPCLIENT, libosmo-mgcp-client >= 1.9.0)

    Please also make sure that both: […]

    Done

• File src/hnbgw.c:

  • Patch Set #2, Line 376: LOGL_DEBUG

    Why debug? All other categories are set to LOGL_NOTICE.

    I have forgotten to switch that back to LOGL_NOTICE

  • Patch Set #2, Line 379:

    ws

    Done

• File src/hnbgw_cn.c:

  • Patch Set #1, 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?

    Yes, see line 542, sccp_sap_up() is the caller.

  • Patch Set #1, 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: […]

    Ack

• File src/hnbgw_cn.c:

  • Patch Set #2, Line 349: uint8_t *msg_data;

    const

    Done

  • Patch Set #2, Line 362: /* Intercept RAB Assignment Request, Setup MGW FSM */

    I am not saying the current approach is wrong, just an alternative variant: […]

    Done

• File src/hnbgw_rua.c:

  • Patch Set #1, Line 232: /* TODO: Check for map and if this is a RAB Assignment response

    what about this?

    Thats done. I will remove it.

  • Patch Set #1, 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 extendin […]

    Done

• File src/hnbgw_rua.c:

  • Patch Set #2, Line 192: uint8_t *msg_data;

    const

    Done

• File src/hnbgw_vty.c:

  • Patch Set #1, 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 […]

    Done

• File src/mgw_fsm.c:

  • Patch Set #1, Line 69: MGW_ST_CRCX_HNB,

    WAIT_CRCX?

    It executes the CRCX on enter. I have used the onenter actions here in this FSM to make the FSM more "direct". In the end the state does the complete CRCX - executing it on enter, waiting for the response -> next state.

  • Patch Set #1, Line 71: MGW_ST_MDCX_HNB,

    WAIT_MDCX?

    (same as above)

  • Patch Set #1, Line 86: uint8_t rab_ass_req[IUH_MSGB_SIZE];

    Use "struct *msgb" here and mgcp_copy()?

    rua_tx_dt() takes a pointer to the data and the length as parameters. I don't see an advantage to set up an msgb here.

  • Patch Set #1, Line 87: unsigned int rab_ass_req_len;

    You don't need this field if you copy the msgb.

    Yes, sure, but its simpler this way.

  • Patch Set #1, Line 190:

    unneeded space

    Done

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

    Done

  • Patch Set #1, Line 414: .in_event_mask = 0

    I find this "0 |" way quite strange and confusing. […]

    Some FSMs are use this style some don't. https://git.osmocom.org/osmo-bsc/tree/src/osmo-bsc/lchan_fsm.c#n1527 The one with the 0 is probably less common.

  • Patch Set #1, Line 485: .allstate_event_mask = 0 | S(MGW_EV_MGCP_TERM)

    Please clean up this line(s).

    Done

  • Patch Set #1, Line 485: .allstate_event_mask = 0 | S(MGW_EV_MGCP_TERM)

    Please clean up this line(s).

    Done

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

    You can probably drop this variable.

    theoretically yes, but in the current code its needed.

• File src/mgw_fsm.c:

  • Patch Set #2, Line 189: switch (event) {

    weird formatting

    Done

• File src/tdefs.c:

  • Patch Set #1, Line 20: {.T = -1001,.default_val = 5,.desc = "Timeout for HNB side call-leg (to-HNB) creation" },

    missing space after comma in all lines

    Done

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