Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13137 )
Change subject: large refactoring: support inter-BSC and inter-MSC Handover ...................................................................... Patch Set 9: (28 comments) hmm, my idea was to have received this review a lot earlier. I am N times through final-final and absolutely final testing after patch changes. I appreciate the review, but to be honest I personally need to get this load off my shoulders. I am applying your review thus far, but would prefer to not squash more modifications into this patch set, but rather move on to separate follow-up patches after this. This patch being in limbo is a huge burden: - having to continuously rebase it keeps introducing merge conflicts that are completely loaded onto my shoulders -- I have to understand and adjust to all other work going on; this limbo has been going on for far too long and by now is profoundly impacting my private life (stress / frustration). - Secondly, squashing changes into the patch set happens sort of hidden, not seen in the git history; I've often had to revisit the reflog to analyse past versions of the patch set to fix bugs introduced by modifications squashed into this -- that's not how version control should happen. So please, unless you have absolutely critical bugs that need fixing, let's move to merge + separate patches as soon as possible. Nevertheless: many thanks for reviewing this with close scrutiny! If you can, by all means please go on to do so! https://gerrit.osmocom.org/#/c/13137/9/include/osmocom/msc/Makefile.am File include/osmocom/msc/Makefile.am: https://gerrit.osmocom.org/#/c/13137/9/include/osmocom/msc/Makefile.am@9 PS9, Line 9: gsm_04_11.h \ > Looks like a meaningless change. I think I decided to alphabetically sort (vim ':sort' command) ... can we just keep this? https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_roles.h File include/osmocom/msc/msc_roles.h: https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_roles.h@379 PS6, Line 379: struct an_apdu { : /* accessNetworkProtocolId */ : enum osmo_gsup_access_network_protocol an_proto; : /* signalInfo */ : struct msgb *msg; : /* If this AN-APDU is sent between MSCs, additional information from the E-interface messaging, like the : * Handover Number, will placed/available here. Otherwise may be left NULL. */ : const struct osmo_gsup_message *e_info; : }; > hmm, indeed, didn't see that... […] In the end decided against using the msgb->cb[], because it makes the type conversions a lot more complex / ugly. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c File src/libmsc/call_leg.c: https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@34 PS9, Line 34: LOG_CALL_LEG > LOGPFSML can handle fi=NULL, so we probably don't need this macro. indeed it ended up being a simplistic wrapper, but it is my preferred pattern to keep FSM specific log macros. That way we can trivially tweak all log context for a given object in a single place. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@78 PS9, Line 78: fi > AFAICS, you're using 'fi' as the talloc-parent in order to deallocate it > automatically when the 'fi' […] I want the struct and the fi to be one talloc unit, and I specifically do not want to have to remember to talloc_free in a cleanup function. It is also a functional requirement to keep the struct around until the FSM instance frees to avoid all sorts of complex freeing cascades hitting use-after-free problems. See http://git.osmocom.org/libosmocore/commit/?id=1f9cc018618e7d1e9a7a37e1ef08e059a4e02e87 -- which would be quite useless if the cleanup function freed the struct separately. You will see this exact pattern in *all* of the FSM instance implementations I have written, both in osmo-bsc and here. I appreciate your opinion, but will not adjust this pattern. I don't understand what you mean by "no need to init dummy call_leg_fsm on line #59." -- line 59 has the global FSM definition, which is not the FSM instance that is allocated here based on that singleton FSM definition. Again this is a common pattern. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@78 PS9, Line 78: talloc_zero > Since you're doing *cl = (struct call_leg) { ... […] you mean I don't need to talloc_zero()? Below cl = (struct...){...} can be seen as syntactic sugar to not have to repeat cl-> in every line, and to make clear that the struct is fully initialized. The talloc API does not provide a macro similar to talloc_zero() that names the struct but doesn't zero initialize. What is the reason for this statement, optimization? I would just keep it as is, no point in changing it? https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@101 PS9, Line 101: cl->fi > Looks like a bug to me. […] osmo_fsm_inst_change_reparent() indeed contains this bug. Thanks for highlighting, I found it while neck deep in developing and forgot about it. Opened https://osmocom.org/issues/3983 Since we need an osmo_fsm_inst_change_parent2() for compat, this code can be merged as-is now, and can be fixed once the new osmo_fsm_inst_change_parent2() exists. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@142 PS9, Line 142: > ws the blank line is intentional https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@145 PS9, Line 145: for (i = 0; i < ARRAY_SIZE(cl->rtp); i++) { > So here we check whether all RTP connections of this call leg are > established, right? Would be great […] IMHO the code is quite clear, but ok. What you see as simplification is code duplication in my eyes. I prefer to traverse arrays in for loops, even if they have only two members. We don't suppress. Incoming event is an RTP stream saying it is ready, and if all are ready we pass on the *CALL_LEG* event that the entire call leg is established. No logging needed besides the events that are already logged. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@153 PS9, Line 153: if (cl->fi->state != CALL_LEG_ST_ESTABLISHED) > This check could be done before iterating over 'cl->rtp': […] I don't remember why I added this if()... theoretically, if any call leg is non-established, the call leg should also not be in established state. I think I would rather remove the if() and have some error log when re-entering the same state... https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@158 PS9, Line 158: rtps = data; > This assignment looks useless to me. You could pass 'data' directly to > osmo_fsm_inst_dispatch(). it is to clarify the type of the data argument. Otherwise the reader has to move to other parts of the code to investigate what the type is intended to be. It is a common pattern that indeed doesn't have much functional use in this case, yet I would prefer to keep it. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@178 PS9, Line 178: struct call_leg *cl = fi->priv; > Same here, just pass 'fi->priv' directly to osmo_fsm_inst_dispatch(). […] same humble opinion, would prefer to keep for some degree of "type safety" https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@211 PS9, Line 211: {} > I know that you prefer this way of array-termination, but in general we use { > 0, NULL }. […] I am being consistent with using {} everywhere in my patches all over the place for years :P The cat is already out of the bag. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@229 PS9, Line 229: call_leg_fsm_establishing_established > The function name looks confusing... […] one function for both states https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@244 PS9, Line 244: /* same action function as above */ > Ok, now I see. […] it is mostly for logging https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@248 PS9, Line 248: in_event_mask > Since we terminate the FSM instance as soon as we enter this state, is there > a chance that it would […] of course, cleanup and termination events ricocheting often dispatch numerous events https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@253 PS9, Line 253: /* same action function as above */ > Copy-pasted ;) thx https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@257 PS9, Line 257: static struct osmo_fsm call_leg_fsm > This symbol was already defined at line #59. it obviously has to be done exactly this way, unless I move the alloc function to below here. I prefer it in the beginning though. It's called "forward declaration" and is a common tool in the C language. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@290 PS9, Line 290: call_leg_ensure_rtp_alloc > Basically, this function just calls call_leg_rtp_alloc() and checks if the > allocation was successful […] the critical difference is that it calls call_leg_rtp_alloc() only if it hasn't been called yet on that rtp_stream. I would like to keep this. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c File src/libmsc/e_link.c: https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c@77 PS9, Line 77: *e = (struct e_link) { > I think you're abusing this way of structure initialization here. […] I prefer this to clearly indicate that the struct is initialized from scratch. You are right that it seems unnecessary, but nevertheless is a common patter I use everywhere: foo = talloc *foo = (struct foo){ initialze all members that can be initialized inline like this } initialize all members that need multiple lines and function calls, like memcpy and so forth what's the point of changing this... https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c@84 PS9, Line 84: memcpy > If 'remote_name' were of type 'const char *', you could just use > osmo_strdup(). […] it is a blob with length, to one day support Global Title or something. Wasn't my idea, but it is a BLOB, not a char*. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c@105 PS9, Line 105: enum msc_role from_role > Unused parameter? indeed, came from an earlier patch set where the role was reflected in the GSUP_ENTITY. Now we have just the GSUP_MESSAGE_CLASS https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c@127 PS9, Line 127: strlen(local_msc_name) > Do we need to also include '\0'? yes :/ it's a long story... https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c@132 PS9, Line 132: if (vsub) > AFAIR, IMSI is mandatory for all GSUP messages. […] the idea is that e_prep_gsup_msg() is generally working. If the result is an invalid message, tough luck for the caller, but I definitely want to avoid null pointer access. The caller might be aware of a missing vsub and obtain the IMSI otherwise? (not sure if any such caller exists, but that is the semantic idea of this function) https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c@156 PS9, Line 156: DEBUGP > How about LOG_MSC_A_CAT() here? yes. This is legacy logging, not sure if we really need to change all of it now? I agree that this can be changed but will not spend time on it. Feel free to post a follow-up patch. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c@279 PS9, Line 279: switch > if (gsm48_hdr_msg_type(gh) != GSM48_MT_RR_PAG_RESP) […] no: one day another message type comes along and we need to change to a switch() again. Following same pattern as above. https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c@409 PS9, Line 409: msc_a_put(msc_a, __func__); > Shouldn't we also msc_a_put(MSC_A_USE_LOCATION_UPDATING) here? the MSC_A_USE_LOCATION_UPDATING is put() by the FSM instance https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c@719 PS9, Line 719: sizeof(struct gsm48_service_request*) > Wait, sizeof pointer?!? Looks like a bug. hmmm this bug predates this patch, feel free to fix separately https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/gsm_04_08.c@1302 PS9, Line 1302: cm_service_type > Unused? defined by vlr.ops API, even if we don't use it, we comply with that cb signature -- To view, visit https://gerrit.osmocom.org/13137 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27e4988e0371808b512c757d2b52ada1615067bd Gerrit-Change-Number: 13137 Gerrit-PatchSet: 9 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-CC: Harald Welte <[email protected]> Gerrit-CC: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Tue, 07 May 2019 23:39:56 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
