Change in osmo-msc[master]: large refactoring: support inter-BSC and inter-MSC Handover

Neels Hofmeyr gerrit-no-reply at
Tue May 7 23:39:56 UTC 2019

Neels Hofmeyr has posted comments on this change. ( )

Change subject: large refactoring: support inter-BSC and inter-MSC Handover

Patch Set 9:


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!
File include/osmocom/msc/
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?
File include/osmocom/msc/msc_roles.h:
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.
File src/libmsc/call_leg.c:
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.
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 -- 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.
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?
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.


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.
PS9, Line 142: 
> ws
the blank line is intentional
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.
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...
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.
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"
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.
PS9, Line 229: call_leg_fsm_establishing_established
> The function name looks confusing... […]
one function for both states
PS9, Line 244: /* same action function as above */
> Ok, now I see. […]
it is mostly for logging
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
PS9, Line 253: /* same action function as above */
> Copy-pasted ;)
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.
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.
File src/libmsc/e_link.c:
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...
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*.
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
PS9, Line 127: strlen(local_msc_name)
> Do we need to also include '\0'?
yes :/ it's a long story...
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)
File src/libmsc/gsm_04_08.c:
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.
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.
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
PS9, Line 719: sizeof(struct gsm48_service_request*)
> Wait, sizeof pointer?!? Looks like a bug.

this bug predates this patch, feel free to fix separately
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
To unsubscribe, or for help writing mail filters, visit

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 <nhofmeyr at>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at>
Gerrit-CC: Harald Welte <laforge at>
Gerrit-CC: Pau Espin Pedrol <pespin at>
Gerrit-Comment-Date: Tue, 07 May 2019 23:39:56 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list