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

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


Vadim Yanitskiy 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: Code-Review-1

(22 comments)

https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_bsc_ho.msc
File doc/sequence_charts/inter_bsc_ho.msc:

https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_bsc_ho.msc@27
PS9, Line 27: 	
ws


https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_msc_ho.msc
File doc/sequence_charts/inter_msc_ho.msc:

https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_msc_ho.msc@47
PS9, Line 47: 	
ws


https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_msc_ho.msc@74
PS9, Line 74: 	
ws


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.


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.


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' 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.


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) { ... } below, zero-initialization doesn't make sense I think.


https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@101
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.


https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@142
PS9, Line 142: 
ws


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 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]))
    break;
  if (!rtp_stream_is_established(cl->rtp[1]))
    break;

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.


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':

  if (cl->fi->state == CALL_LEG_ST_ESTABLISHED)
    break;


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().


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(). Casting from (void *) to (struct call_leg *) and then back to (void *) doesn't make sense.


https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@192
PS9, Line 192: 
ws


https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@202
PS9, Line 202: break
Should we OSMO_ASSERT() here?


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 }. Not critical, but let's please be consistent.


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... Establishing established?


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. But still, it looks odd that we have two different states with the same set of events, state transitions and the same event handler...


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 receive any events?


https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@253
PS9, Line 253: /* same action function as above */
Copy-pasted ;)


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.


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. Looks redundant to me.



-- 
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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-CC: Pau Espin Pedrol <pespin at sysmocom.de>
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: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190507/d38b55ed/attachment.html>


More information about the gerrit-log mailing list