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

Vadim Yanitskiy gerrit-no-reply at
Tue May 7 21:11:51 UTC 2019

Vadim Yanitskiy has posted comments on this change. ( )

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

Patch Set 9: Code-Review-1

File doc/sequence_charts/inter_bsc_ho.msc:
PS9, Line 27: 	
File doc/sequence_charts/inter_msc_ho.msc:
PS9, Line 47: 	
PS9, Line 74: 	
File include/osmocom/msc/
PS9, Line 9: gsm_04_11.h \
Looks like a meaningless change.
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.
PS9, Line 78: fi
AFAICS, you're using 'fi' as the talloc-parent in order to deallocate it automatically when the 'fi' is terminated and freed. If it's the only reason, then I don't like this approach. You could just call talloc_free(fi->priv) in the FSM's cleanup() handler. Thus there will be no need to init dummy call_leg_fsm on line #59.
PS9, Line 78: talloc_zero
Since you're doing *cl = (struct call_leg) { ... } below, zero-initialization doesn't make sense I think.
PS9, Line 101: cl->fi
Looks like a bug to me. We need to change the parent talloc context of 'cl' itself, right? Here we keeping 'cl' as a child of the old 'osmo_fsm_inst', and changing the talloc-parent of 'new_parent_fi' to 'new_parent_fi' o_O.
PS9, Line 142: 
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 to have a comment.

Also, is it possible to have more than 2 RTP connections for a call leg? If no, this code can be simplified:

  if (!rtp_stream_is_established(cl->rtp[0]))
  if (!rtp_stream_is_established(cl->rtp[1]))

And should we print some debug / notice message if not all RTP connections are established? Currently we silently suppress CALL_LEG_EV_RTP_STREAM_ESTABLISHED in such case.
PS9, Line 153: if (cl->fi->state != CALL_LEG_ST_ESTABLISHED)
This check could be done before iterating over 'cl->rtp':

  if (cl->fi->state == CALL_LEG_ST_ESTABLISHED)
PS9, Line 158: rtps = data;
This assignment looks useless to me. You could pass 'data' directly to osmo_fsm_inst_dispatch().
PS9, Line 178: struct call_leg *cl = fi->priv;
Same here, just pass 'fi->priv' directly to osmo_fsm_inst_dispatch(). Casting from (void *) to (struct call_leg *) and then back to (void *) doesn't make sense.
PS9, Line 192: 
PS9, Line 202: break
Should we OSMO_ASSERT() here?
PS9, Line 211: {}
I know that you prefer this way of array-termination, but in general we use { 0, NULL }. Not critical, but let's please be consistent.
PS9, Line 229: call_leg_fsm_establishing_established
The function name looks confusing... Establishing established?
PS9, Line 244: /* same action function as above */
Ok, now I see. But still, it looks odd that we have two different states with the same set of events, state transitions and the same event handler...
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 receive any 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.
PS9, Line 290: call_leg_ensure_rtp_alloc
Basically, this function just calls call_leg_rtp_alloc() and checks if the allocation was successful. Looks redundant to me.

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 21:11:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list