This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes