Patch Set 4: Code-Review-1 (21 comments)
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. Line 28: /* A human readable name to display in the logs */ does it make sense to have a name inside the fms, and a name here? I would think it only makes sense in cases where fsm==NULL, but is that a valid use case? Line 36: struct gsm_network *network; I don't think we want to keep an extra pointer to the network around in every mgcp_ctx? Line 38: int chan_mode; is this really an integer type, don't we have some enum for this? PS4, Line 39: f could be a bool? https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_bssap.c File src/osmo-bsc/osmo_bsc_bssap.c: Line 582: new whitspace error? Line 767: new whitspace error? Line 772: LOGP(DMSC, LOGL_NOTICE, "Sending assignment complete message...\n"); sending the assignment complete is an normal event, i.e. LOGL_DEBUG seems applicable, not notice. Line 787: LOGP(DMSC, LOGL_ERROR, "Failed to generate assignment completed message!\n"); \ there is no context in this message at all. In a loaded network, we may be processing dozens to hundreds of assignment complete in a second. How does the operator make sense of this log message, without any context on which call it relates to? Same applies to other log messages like the one above. 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) Line 287: new whitespace bug https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_mgcp.c File src/osmo-bsc/osmo_bsc_mgcp.c: Line 137: LOGP(DMSC, LOGL_NOTICE, "MGCPGW: (%s) fsm-state: %s\n", * logging inside fsm's should use LOGPFSM in order to automatically print the prefix with the fsm identity * the state of a fsm is logged at every incoming event and every state transmission by the fsm core. what does your message add to that? * LOGL_NOTICE why? because it's an error? but then, the error is logged below. Line 173: LOGP(DMSC, LOGL_NOTICE, * all this logging is implemented by the FSM core *and* it is not a NOTICE event, is it? Line 257: LOGP(DMSC, LOGL_NOTICE, LOGPFSM whenever you have a fsm_inst as context, all over this file. Line 271: LOGP(DMGCP, LOGL_NOTICE, "MGCPGW: (%s) MGCPGW proceeding assignment request...\n", mgcp_ctx->name); another NOTICE. Please be more careful when deciding on the log level. This is clearly not something that should be brought to the attention of the operator/sysadmin during normal operation. Line 499: LOGP(DMGCP, LOGL_ERROR, "MGCPGW: (%s) Cannot parse CRCX response\n", mgcp_ctx->name); when we encounter errors in parsing MGCP, shouldn't we always return some kind of information to the originator of the message? I don't know RFC3435 by heart, but normally most protocols require error messages on parsing erros? Maybe not because this is already a response? Line 780: .name = "FSM MGCP", I'm not sure if spaces are permitted here in the name. This will again cause problems with exporting its properties over the ctrl interface. Please follow naming of other FSMs. Also, might be good to indicate it's a FSM for the call agent side, not to be confused with the gateway side. Line 806: if (osmo_fsm_find_by_name(fsm.name) != &fsm) this is a linear list traversal over all FSMs in the program at each processing of an assignment request. I would much rather prefer the code to explicitly call osmo_fsm_register() once. Or you keep a global static variable here, which you can check if you registered the fsm before. thanks. Line 813: snprintf(mgcp_ctx->name, sizeof(mgcp_ctx->name), "MGCP FSM, id=%i", conn->conn_id); see my other comment, if we really need the FSM name and another name in the wrapper around the fsm. https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_vty.c File src/osmo-bsc/osmo_bsc_vty.c: Line 977: new whitespace bug Line 1043: new whitespace bug -- 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 <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
