Patch Set 2: Code-Review-1 (31 comments)
https://gerrit.osmocom.org/#/c/4980/2//COMMIT_MSG Commit Message: Line 15: Depends osmo-mgw: Iab6a6038e7610c62f34e642cd49c93d11151252c please use the tag as "Depends: foo" to match general tag syntax like below ... I usually use Depends: Iab6a6038e7610c62f34e642cd49c93d11151252c (osmo-mgw) and don't separate from the other tags, i.e. drop the blank line https://gerrit.osmocom.org/#/c/4980/2/include/osmocom/msc/msc_ifaces.h File include/osmocom/msc/msc_ifaces.h: Line 44: int msc_iu_rab_act_cs(struct gsm_trans *trans); -1: msc_ifaces.h/c didn't really turn out as clearly as I had intended... But looking at this change I find that this function should be in iucs.h/c. Let's not add msc_ to the name and just add iu_rab_act_cs to iucs.h so that you can call it from the FSM code. Another patch can move the implementation from msc_ifaces.c to iucs.c (or this patch if you like) https://gerrit.osmocom.org/#/c/4980/2/include/osmocom/msc/msc_mgcp.h File include/osmocom/msc/msc_mgcp.h: Line 33: char conn_id_cn[MGCP_CONN_ID_MAXLEN]; ?: I wonder whether rtp_endpoint and the conn_ids should rather be part of struct mgcp_client? How does osmo-bsc solve it, does it have the same structure that can be unified between osmo-bsc and osmo-msc, i.e. in the mgcp_client API? https://gerrit.osmocom.org/#/c/4980/2/src/libmsc/msc_ifaces.c File src/libmsc/msc_ifaces.c: Line 194: return msc_mgcp_call_assignment(trans); looks like callers should just call msc_mgcp_call_assignment() directly. Line 249: msc_mgcp_call_release(trans); looks like callers should just call msc_mgcp_call_release() directly. https://gerrit.osmocom.org/#/c/4980/2/src/libmsc/msc_mgcp.c File src/libmsc/msc_mgcp.c: Line 47: enum int_cause_code { -1: prefer msc_mgcp_ instead of int_ because if you follow this scheme, we will have int_ prefixes everywhere and it is harder to grep the code Line 59: static const struct value_string int_cause_codes_str[] = { -1: typical naming would be foo_names like you use below, so msc_mgcp_cause_code_names[] Line 93: enum fsm_evt { -1: again you are using the same general struct name, just 'fsm_', like in osmo-bsc. I know it's a static context, but please give each FSM its own unique naming, like my patches that we merged into your osmo-bsc FSM patches. Line 126: static const struct value_string fsm_evt_names[] = { fsm_ Line 217: get_value_string(fsm_bsc_mgcp_state_names, fi->state), get_value_string(fsm_evt_names, event)); -1: again, you do not need to log FSM states or events, the FSM code does that. I've mailed you so before, let's not repeat this. Line 222: "CRCX/RAN: creating connection for the RAN side on " "MGW endpoint:%x...\n", mgcp_ctx->rtp_endpoint); -1: best use "0x%x" so we always know it is logged in hex. Are you sure it should be in hex though? In the VTY rtp endpoint ranges it isn't. same multiple times below. Line 240: LOGPFSML(fi, LOGL_DEBUG, "CRCX/RAN: transmitting MGCP message to MGW...\n"); (wondering whether this log line adds any info to the log, since the FSM already logged ST_CRCX_RAN. same multiple times below) Line 248: return; lol, return at the end of a void function? (some more below) Line 319: get_value_string(fsm_bsc_mgcp_state_names, fi->state), get_value_string(fsm_evt_names, event)); nope Line 422: get_value_string(fsm_bsc_mgcp_state_names, fi->state), get_value_string(fsm_evt_names, event)); nope Line 442: handle_error(mgcp_ctx, MGCP_ERR_ASSGMNT_FAIL); (code dup: could have one handle_error(...) below and goto assignment_fail from each failure) Line 460: /* Note: When we reach this point than the situation is basically that (then) Line 465: osmo_fsm_inst_state_chg(fi, ST_MDCX_CN, 0, 0); ?: no timeout? Line 492: get_value_string(fsm_bsc_mgcp_state_names, fi->state), get_value_string(fsm_evt_names, event)); ... Line 503: "MDCX/CN: completing connection for the CN side on " "MGW endpoint:%x...\n", mgcp_ctx->rtp_endpoint); (line break or drop the " " in the middle) Line 507: conn->rtp.remote_port_cn); (rather combine with preceding LOGPFSML for less log lines and a bit less logging overhead) Line 552: OSMO_ASSERT(conn); you're copying these lines and local variables around everywhere, even in functions where you don't use them at all, like this one, which is only using mgcp_ctx? Line 609: conn->rtp.remote_port_ran); (combine logs) Line 691: LOGPFSML(fi, LOGL_ERROR, "call active, waiting for teardown...\n"); really an error? Line 803: .in_event_mask = (1 << EV_INIT), (we often use an S() macro, e.g. see top of vlr_lu_fsm.c and struct sub_pres_vlr_states definition) Line 805: .name = "ST_CRCX_RAN", (I like to use OSMO_STRINGIFY(ST_CRCX_RAN) because then there won't be typos ... up to you) Line 877: /* Notify that the a new call begins. This will create a connection for the "the a" Line 895: LOGP(DMGCP, LOGL_ERROR, "invalid conn, call assignment failed\n"); can't we log some context? Remember Holger's scenario ... it is 37c3, you have a couple thousand subscribers... Line 927: mgcp_ctx->fsm = osmo_fsm_inst_alloc(&fsm_msc_mgcp, NULL, NULL, LOGL_DEBUG, name); I think it should be a child of the conn->conn_fsm. Line 954: LOGP(DMGCP, LOGL_ERROR, "invalid remote call leg address, call completion failed\n"); context please in all of these logs. Line 961: if (!trans) { In general, you could have a look whether any code path leading to these checks possibly *can* have a NULL transaction, and so forth. Same in some other instances above. In most code, we assume that the structs passed in are valid and avoid a lot of bloat that way. -- To view, visit https://gerrit.osmocom.org/4980 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538 Gerrit-PatchSet: 2 Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
