Patch Set 4: Code-Review-1 (35 comments)
phew, that was a lot to review. I hope you're not overwhelmed by the amount of comments, after all it's just a (big) bunch of details. FYI, I made some FSM graphs for this patch using the mad libosmocore/contrib/fsm-to-dot.py script I wrote last year: http://people.osmocom.org/neels/bsc_fsms/ (I needed to tweak a few things for it to work, will submit those tweaks separately) https://gerrit.osmocom.org/#/c/4334/4/configure.ac File configure.ac: Line 52: PKG_CHECK_MODULES(LIBOSMOLEGACYMGCP, libosmo-legacy-mgcp >= 1.0.0) (would have been nicer to keep it on the same line for diff reading) Line 138: src/osmo-bsc/Makefile is removing osmo-bsc_nat intentional? I didn't read it in the commit log. https://gerrit.osmocom.org/#/c/4334/4/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: Line 484: } mgw; (your usual impulse should be to extend structs only at their end ... but since it's not public API, I guess it's fine) https://gerrit.osmocom.org/#/c/4334/4/include/osmocom/bsc/osmo_bsc.h File include/osmocom/bsc/osmo_bsc.h: Line 34: struct sockaddr_storage aoip_rtp_addr_remote; (do our previous preference of char* for IP addresses apply here as well?) https://gerrit.osmocom.org/#/c/4334/4/include/osmocom/bsc/osmo_bsc_mgcp.h File include/osmocom/bsc/osmo_bsc_mgcp.h: PS4, Line 1: Sysmocom s.f.m.c. > sysmocom awalys lower-case and "-" before s.f.m.c. oh ... I (neels) didn't know about the dash, which means that all headers I (c)'d like this have "sysmocom s.f.m.c." now. Line 36: struct gsm_network *network; > I don't think we want to keep an extra pointer to the network around in eve AFAICT all you need is network->mgcp.client, rather just place that one here, and pass it to mgcp_assignm_req() instead of a gsm_network*. https://gerrit.osmocom.org/#/c/4334/4/src/Makefile.am File src/Makefile.am: Line 35: osmo-bsc \ (remove osmo-bsc_nat?) https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_audio.c File src/osmo-bsc/osmo_bsc_audio.c: Line 94: (unrelated cosmetic fix) https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_bssap.c File src/osmo-bsc/osmo_bsc_bssap.c: Line 341: /* NOTE: This is the SCCP-Lite case, since we do not handle does this version of osmo-bsc even support SCCPlite anymore? I thought the SCCPlite one has to be built from openbsc.git, since adding the new sigtran has modified libbsc. Same applies to other sccplite if cases ... have you tested them and I got something wrong? IIUC having both AoIP and SCCPlite support in the same program means that we add SCCPlite to the new SIGTRAN, before that we simply will not? Line 564: conn->mgcp_ctx = mgcp_assignm_req(NULL, msc->network, conn, chan_mode, full_rate); don't pass a NULL talloc ctx!! We don't want to create rogue root contexts, it shall be a tree structure. 'conn' might be a good ctx. Line 768: OSMO_ASSERT(lchan->abis_ip.ass_compl.valid == true); ('== true' is superfluous) Line 772: LOGP(DMSC, LOGL_NOTICE, "Sending assignment complete message...\n"); > sending the assignment complete is an normal event, i.e. LOGL_DEBUG seems a (if it happens rarely and is interesting, LOGL_INFO is also available. NOTICE is almost as important as ERROR.) https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_main.c File src/osmo-bsc/osmo_bsc_main.c: Line 284: printf("MGCPGW connect failed\n"); > useful to print the connect to *where* has failed (IP/port) (maybe just "MGCP" without "GW" is better? "MGCPGW" is a term that was used elsewhere in a temporary way, now replaced by "MGW") https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_mgcp.c File src/osmo-bsc/osmo_bsc_mgcp.c: Line 37: #define MGCP_MGW_TIMEOUT 4 /* sek */ in english, it would be "sec", but rather write out "in seconds" to make it clear. Line 40: #define MGCP_BSS_TIMEOUT_TIMER_NO 7412 ("TIMER_NO" to me reads like "Timer? No!" .. make this "TIMER_NR"?) Line 46: enum fsm_states { shouldn't below fsm_state_names[] make the comments here unnecessary? At least they shouldn't mismatch with below strings, which they do. Line 71: {ST_ASSIGN_PROC, "ST_ASSIGN_PROC (conntinue assingment)"}, "continue", "assignment" Line 74: {ST_ASSIGN_COMPL, "ST_ASSIGN_COMPL (complete assingment)"}, "assignment" Line 76: {ST_HALT, "ST_HALT (destroy state machine)"}, nice, we get both the actual state constant name in the log as well as a human understandable description. Line 77: {0, NULL}, (could omit the comma, nothing should ever follow the end marker) Line 121: /* On error, go directly to the DLCX phase. */ (the comment says the identical thing that the function name already does) Line 122: static void on_error_goto_dlcx(struct mgcp_ctx *mgcp_ctx) this is called from pretty much all the code paths; it might be nice to add an error cause message arg that this function then logs in its LOGPFSM()? Line 125: * behave like the call were ended normally. */ If this comment is useful and applies to the entire function, it should go above where the API doc usually is. Line 130: OSMO_ASSERT(mgcp_ctx); (usually it makes sense to assert non-NULL once, early in the code path, instead again and again in each static utility function, which is just bloating the code; especially when NULL is never a permitted situation. Notably, it can make sense to do this in all event callback functions where we're not sure what we might be getting.) Line 143: osmo_fsm_inst_state_chg(fi, ST_DLCX, 0, 0); should this have a timeout? Line 185: /* Generate MGCP message string */ I usually like this notation: mgcp_msg = (struct mgcp_msg){ .verb = MGCP_VERB_CRCX, .presence = ... .call_id = ... }; Line 190: snprintf(mgcp_msg.endpoint, MGCP_ENDPOINT_MAXLEN, MGCP_ENDPOINT_FORMAT, rtp_endpoint); I think this snprintf should move into mgcp_msg_gen() and make mgcp_msg.endpoint a uint16_t? does that make sense, or can the endpoint be an arbitrary string and uint16_t is just a local limitation? Line 193: mgcp_msg.conn_mode = MGCP_CONN_LOOPBACK; (would it make sense to add thin API functions like mgcp_msg_set_foo() that do msg.foo = foo as well as presence |= MGCP_FOO? ... might end up bloaty, not sure, just an idea.) Line 199: mgcp_client_tx(mgcp, msg, crcx_for_bts_resp_cb, mgcp_ctx); admitted, we have logging in mgcp_client_tx, but would still be nice to do some LOGPFSM() to notice the important event in the FSM flow. And then we might as well go to an error state directly instead of waiting for the timeout first. Line 207: struct mgcp_ctx *mgcp_ctx = (struct mgcp_ctx *)priv; (is a cast from void* even necessary? I think it's not warned about, is it?) Line 250: int full_rate; (also bool according to Harald's earlier comment?) Line 282: /* Forward declaration to keep the function in logical order */ (kind of obsolete comment) Line 780: .name = "FSM MGCP", > I'm not sure if spaces are permitted here in the name. This will again caus (I'd drop the "FSM " part, we will see in logging that it is an FSM anyway) Line 814: mgcp_ctx->fsm = osmo_fsm_inst_alloc(&fsm, NULL, ctx, LOGL_DEBUG, "FSM MGCP INST"); this string should also be without spaces, again for ctrl interface. https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_vty.c File src/osmo-bsc/osmo_bsc_vty.c: Line 976: struct gsm_network *net = bsc_gsmnet; why create a local var if it is used only once and also is just an alias for another short enough name? -- To view, visit https://gerrit.osmocom.org/4334 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2882b7ca31a3219c676986e85045fa08a425d7a Gerrit-PatchSet: 4 Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
