This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Vadim Yanitskiy gerrit-no-reply at lists.osmocom.orgVadim 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/a8f74481/attachment.htm>