Patch Set 11: Code-Review-1 (31 comments)
In general, I wonder whether it is helping that I comment on the numerous cosmetic issues. Most of it could be caught by you reading your own patch, it is taking a lot of time of my day and it seems like I am reiterating similar obvious points over and over. It makes me feel like a nitpick and probably makes you feel unappreciated, also clutters the really important code review and probably makes me miss the real problems; IOW, I would enjoy to give more constructive feedback so that we can both feel good about the review. Either by me letting every cosmetic no-go pass uncommented or by you fixing them before submitting a patch; not sure which it should be, maybe a bit of both; maybe others have an opinion on that too. I'd like to finally see this merged, but still can't +2 because: - I don't understand the FSM -- it should be quite linear, but it reads like it has paths crossing and teardown is skipping/merging some steps that should be separate. If the FSM is correct and I'm not seeing it, then the comments and state/event naming could help clarifying; see inline comments. - missing inet_addr() error handling / needs replacement. https://gerrit.osmocom.org/#/c/4980/11/include/osmocom/msc/msc_mgcp.h File include/osmocom/msc/msc_mgcp.h: Line 26: /* MGCP state handler context (fsm etc..) */ explain the scope? mgcp context for one call leg? one subscriber? Line 31: /* RTP endpoint number */ (omit comments that just mirror the variable name; explain what the endpoint is about or drop the comment?) Line 35: * needed */ use line width instead of wrapping short lines https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/a_iface.c File src/libmsc/a_iface.c: Line 412: rtp_addr_in.sin_addr.s_addr = inet_addr(conn->rtp.local_addr_ran); needs some error checking. man inet_addr: The inet_addr() function converts the Internet host address cp from IPv4 numbers-and-dots notation into binary data in network byte order. If the input is invalid, INADDR_NONE (usually -1) is returned. Use of this function is problematic because -1 is a valid address (255.255.255.255). Avoid its use in favor of inet_aton(), inet_pton(3), or getaddrinfo(3), which provide a cleaner way to indicate error return. https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: Line 1333: /* Initiate the teadown of the related connections on the MGW */ "teadown" Line 1390: /* helper function for tch_bridge() to bridge the RTP Voice streams also */ (I find "helper function for" is something you could write everywhere; I also used to write this phrase a lot some years ago, but better just say what it does) Line 2691: uint32_t addr = inet_addr(trans->conn->rtp.local_addr_cn); inet_addr, error checking https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/iucs.c File src/libmsc/iucs.c: Line 226: " rtp=%x:%u, use_x213_nsap=%d\n", conn_id, rab_id, rtp_ip, I would have said, use line width, but this is just moving code, right? Line 235: " conn_id=%d rab_id=%d rtp=%x:%u\n", same https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/msc_mgcp.c File src/libmsc/msc_mgcp.c: Line 56: /* Some internal cause codes to indicate fault use line width Line 70: /* Human readable respresentation of the faul codes, line width, also multiple more times below Line 102: * may now forward IP/Port of the remote call leg to the MGW*/ (space before '*/') Line 167: if (fi->T == MGCP_MGW_TIMEOUT_TIMER_NR) { (a lot of "Note:" below) Line 180: osmo_fsm_inst_dispatch(fi, EV_TEARDOWN, mgcp_ctx); could make sense to place teardown in a ST_HALT -> on_enter function? Otherwise could make sense to combine the state_chg + event dispatch in a common function Line 231: "CRCX/RAN: creating connection for the RAN side on " "MGW endpoint:0x%x...\n", mgcp_ctx->rtp_endpoint); join "..." "..." Line 710: osmo_fsm_inst_state_chg(fi, ST_HALT, MGCP_MGW_TIMEOUT, MGCP_MGW_TIMEOUT_TIMER_NR); above, a TEARDOWN event follows after the HALT state_chg. What triggers the teardown here? Line 747: if (mgcp_ctx->free_ctx) { the free_ctx flag essentially creates two kinds of HALT states. One where we're still waiting for something (but what?) and one where we're going to free on any (!) incoming event? I would prefer if the waiting-for-x and halting + freeing states were explicit and distinct states, instead of using the same FSM state split up by additional ctx flag. Also would welcome if it was obvious which kinds of events are expected to arrive here, and that the event is distinct. For example, it looks now like the teardown event could happen in any context, and once it means "go to HALT state" (does it?) and once it means "let's discard the FSM right now"... would be better to have explicit and distinct events. Line 749: talloc_free(mgcp_ctx); mabe clear the free_ctx flag to be safe? otherwise set mgcp_ctx->fsm to NULL? IOW, what is preventing a double free, given that the HALT state could be entered multiple times? Line 761: .out_state_mask = S(ST_HALT) | S(ST_CALL) | S(ST_CRCX_CN), IIUC the next state is CRCX_CN, not ST_CALL? Howcome do all states allow transitioning to ST_CALL? Line 765: /* When the response to the CRCX is received, then proceed with sending "the RAN CRCX" Line 774: * the RAT, this will either trigger an Assignment Request on the RAT? Line 783: * we will enter the MDCX phaseby sending an MDCX for the CN side "phaseby" Line 793: * the assignment is completed */ above it sounds like CRCX_COMPL sends the Assignment Requests and MDCX_CN receives the Assignment Complete. Then, which Assignment Complete state is this? Line 800: /* When the response for the MDCX is received, send the MDCX for the "for the CN MDCX" Line 803: .in_event_mask = S(EV_TEARDOWN) | S(EV_ASSIGN), I find this hard to follow: ST_MDCX_RAN receives EV_ASSIGN, but ST_ASS_COMPL above has already passed, while ST_MDCX_CN coment above says it waits for completion of the assignment request and actually has ST_ASS_COMPL as out_state... Am I missing some pattern? It currently reads to me like reverse / mixed-up. Line 808: /* The MDCX phase is complete when the response is received from the "the RAN side MDCX response" Line 809: * MGW. The call is now active */ when the call is now active, why is this not the ST_CALL state? Oh, you mean this state is *waiting* for the MDCX_RAN_RESP, now I see in the in_event_mask Line 825: * the state machine. */ IIUC there should be DLCX for both the RAN and CN sides, each returning a DLCX OK, and finally a free of the FSM: appears to me like we should actually need three states for tearing down the call. Line 843: /* Notify that that a new call begins. This will create a connection for the "that that" Line 846: * trans: transaction context use punctuation in doc, don't rely on line endings Line 912: /* Inform the FSM that the assignment (RAN connection) is now complete punctuation on all of these lines -- 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: 11 Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
