View Change

15 comments:

• 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?

  • 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:
    if (msg_len > 2) {
     switch (msg_data[1]):
        case RANAP_ProcedureCode_id_RAB_Assignment:
          if (!map->is_ps)
            ...
          break;
    }

• 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?

  • 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 extending this.

• 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 of them.

• File src/mgw_fsm.c:

  • Patch Set #1, Line 69: MGW_ST_CRCX_HNB,

    WAIT_CRCX?

  • Patch Set #1, Line 71: MGW_ST_MDCX_HNB,

    WAIT_MDCX?

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

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

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

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

  • Patch Set #1, Line 190:

    unneeded space

  • 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

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

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

    Please clean up this line(s).

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

    You can probably drop this variable.

• 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

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