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

Harald Welte gerrit-no-reply at
Sun Apr 14 15:29:15 UTC 2019

Harald Welte has posted comments on this change. ( )

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

Patch Set 6:

File include/osmocom/msc/gsup_client_mux.h:
PS6, Line 13: 
could use some comment about what a "GSUP Client Mux" actually is all about.
File include/osmocom/msc/mncc.h:
PS6, Line 200: union mncc_msg {
             : 	uint32_t msg_type;
             : 	struct gsm_mncc signal;
             : 	struct gsm_mncc_hello hello;
             : 	struct gsm_mncc_rtp rtp;
             : 	struct gsm_mncc_bridge bridge;
             : };
why do we have a union of just a few MNCC mesage types? I could understand a union of all the possible options, but why only specificaly those?
File include/osmocom/msc/mncc_fsm.h:
PS6, Line 8: GPL-2.0+
osmo-msc in general is AGPLv3+, why is GPLv2+ sggested here? Is there a plan to move this to a shared library whihc might also be used from non-AGPLv3+ programs?
PS6, Line 35: enum mncc_fsm_event {
I think this file definitely needs quite a lot more comments.  In general the states and events deserve some kind of description, as do the structs.
PS6, Line 64: struct mncc_outgoing_call_req {
the struct has a self-explanatory name, good.
PS6, Line 105: struct mncc {
this one unfortunately not.  "MNCC" is mobile netowkr call control.  So I would expect some kind of global object, but the fact that a single callref is referenced, this appears to be something related to a single call?  This should be expressed in the name of the struct and a comment should describe what the struct is representing.
PS6, Line 125: 	int parent_event_call_setup_complete;
the name seems to hint a true/false property?
File include/osmocom/msc/msc_a.h:
PS6, Line 8: GPL-2.0+
same AGPLv3+ comment
PS6, Line 75: 		} ciphering;
neither AKA nor IMEISV retrieval are strictly speaking part of ciphering.  Just saying.  This isn't bikeshedding, I just want to make sure we use as precise naming as possible to ease readability and avoid any misunderstandings.
PS6, Line 80: 	/* struct msc_role_common must remain at start */
not imporant, but one could have an OSMO_ASSERT on the 'offsetof() == 0' somewhere during program startup.
File include/osmocom/msc/msc_ho.h:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
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;
             : };
as this just adds two 'long' words to 'struct msgb' one could have implemented this in arguably more osmocom-style using the msgb->cb[] array and some accessor macros.  No need to change it, I'm just sharing my thoughts.
File include/osmocom/msc/nas.h:
PS6, Line 111: 		struct geran_encr *chosen_encryption;
the comment explains that "0 means no such IE was present".  But the member is a pointer.  So where's the semantic differenece to chosen_encryption = NULL?
File src/libmsc/call_leg.c:
PS6, Line 64: 	OSMO_ASSERT( osmo_fsm_register(&call_leg_fsm) == 0 );
I would typically try to move as much as possible (particularly fsm registrations) into automatically-called __attribute__((constructor)) functions.  Sure, the 'gsm_network' doesn't exist yet, so not everything can be done like this without larger changes.  Not needed to change now, but we should do that in follow-up patches
File src/libmsc/cell_id_list.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
File src/libmsc/e_link.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 380: Point a struct msgb's data members directly at the data buffer of gsup_msg->an_apdu. This *is* a hack, and the msgb
             :  * returned is a static struct msgb: it must *not* be freed, put in a queue or kept around after gsup_msg->an_apdu is
             :  * gone. The returned msgb can *not* be passed down a RAN peer's SCCP user SAP. This can be useful to "peek" at the
             :  * included data by passing to a nas_decode implementation in a code path that does not pass any PDU on and wants to
             :  * avoid dynamic allocation.
*really* ugly and easy to abuse/introduce bugs.  How often do we do this?  I'd say let's rather make a copy of the msgb  in such situations or find another solution.
File src/libmsc/gsm_04_08.c:
PS6, Line 248: int compl_l3_msg_is_r99(const struct msgb *msg)
comment and function name don't seem to agree on what this function is doing
File src/libmsc/gsm_04_80.c:
PS6, Line 39: subscriber
does msc_a really refference a subscriber?
File src/libmsc/gsup_client_mux.c:
PS6, Line 37: 	LOGP(DLGSUP, LOGL_DEBUG, "No explicit GSUP Message Class, trying to guess from message type %s\n",
            : 	     osmo_gsup_message_type_name(gsup_msg->message_type));
as pretty much all existing GSUP messages by any existing program don't have the message_class set, I'm not sure we should log a DEBUG message for each of them.  Maye in a few  years time, but not today.
File src/libmsc/mncc_fsm.c:
PS6, Line 10:  * SPDX-License-Identifier: GPL-2.0+
File src/libmsc/msc_a.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 983: 	OSMO_ASSERT(osmo_fsm_register(&msc_a_fsm) == 0);
can be moved to __Attribute__((constructor))
File src/libmsc/msc_a_remote.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 364: 	OSMO_ASSERT(osmo_fsm_register(&msc_a_remote_fsm) == 0);
can be moved to __attribute__((constructor))
File src/libmsc/msc_ho.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 65: 	osmo_fsm_register(&msc_ho_fsm);
can be moved to __attribute__((constructor))
PS6, Line 138: 	}
             : 	if (success) {
else ?
File src/libmsc/msc_i.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 310: {
__attribute__((constructor)) avoids having to explicitly call this function and can make it static.
File src/libmsc/msc_i_remote.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 225: 	OSMO_ASSERT(osmo_fsm_register(&msc_i_remote_fsm) == 0);
File src/libmsc/msc_t.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 872: {
File src/libmsc/msc_t_remote.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 204: 	OSMO_ASSERT(osmo_fsm_register(&msc_t_remote_fsm) == 0);
File src/libmsc/msc_vty.c:
PS6, Line 512: DEFUN(cfg_msc_handover_number_range, cfg_msc_handover_number_range_cmd,
MSISDNs typically also have NPI/TON (numbering plan / type of number). probably makes sense to configure those explicitly somewhere and include them in the match?
PS6, Line 522: FIXME leading zeros?? 
Indeded, it probably makes sense not to treat this as unsigned integers but as some kind of "digit prefix". numbers with leading zeroes are very well valid numbers.

One example (in terms of the VTY/UI) is 00123456xxx" where then basically 1000 numbers with a common prefix are allocated as pool for handover numbers.
File src/libmsc/msub.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 152: 	OSMO_ASSERT(osmo_fsm_register(&msub_fsm) == 0);
File src/libmsc/nas.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
File src/libmsc/nas_a.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
File src/libmsc/nas_iu.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
PS6, Line 95: static void nas_iu_decode_l3(struct nas_dec *nas_iu_decode, RANAP_NAS_PDU_t *nas_pdu, const char *msg_name)
PS6, Line 114: static void nas_iu_decode_err(struct nas_dec *nas_iu_decode, RANAP_ErrorIndicationIEs_t *ies)
PS6, Line 123: 							      RANAP_RAB_SetupOrModifiedItemIEs_t *setup_ies)
PS6, Line 125: 	RANAP_RAB_SetupOrModifiedItem_t *item;
these here then subsequently also all const
PS6, Line 173: 	RANAP_IE_t *ranap_ie;
don't we get warnings here? 'ies' is const but the stack variables here not?
PS6, Line 256: static void nas_iu_decode_ranap_msg(void *_nas_dec, ranap_message *message)
ideally the entire input '*message' would be const here, to ensure all downstream users also are 'const'
PS6, Line 416: static void ranap_handle_cl(void *ctx, ranap_message *message)
maybe even here const?
File src/libmsc/neighbor_ident.c:
PS6, Line 8:  *
no SPDX identifier
File src/libmsc/paging.c:
PS6, Line 9: 
mo license header at all
File src/libmsc/ran_infra.c:
PS6, Line 8:  * SPDX-License-Identifier: GPL-2.0+
File src/libmsc/ran_peer.c:
PS6, Line 14: 
license header missing
PS6, Line 19: 	OSMO_ASSERT( osmo_fsm_register(&ran_peer_fsm) == 0);
File src/libmsc/rtp_stream.c:
PS6, Line 12: 
license header missing
PS6, Line 38: 	OSMO_ASSERT(osmo_fsm_register(&rtp_stream_fsm) == 0);
File src/libmsc/sccp_ran.c:
PS6, Line 5:  *
mp SPDX identifier
File src/osmo-msc/msc_main.c:
PS6, Line 83: 2016
at the very least that line here should be updated to 2016-2019 now with those significant changes.

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: 6
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at>
Gerrit-CC: Harald Welte <laforge at>
Gerrit-Comment-Date: Sun, 14 Apr 2019 15:29:15 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list