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

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Sun Apr 14 17:06:09 UTC 2019


Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13137 )

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


Patch Set 6:

(18 comments)

I agree with all license,const comments.
I actually neglected to do a final pass for those things.
Another patch set will follow.

https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc.h
File include/osmocom/msc/mncc.h:

https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc.h@200
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 possib […]
I didn't come across any others? I even added 'hello' and 'bridge' even though they aren't used in this union and thought I had them all... Ah, maybe I excluded the data_frame because we route all RTP via the MGW now? Any others still missing?


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h
File include/osmocom/msc/mncc_fsm.h:

https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@8
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 share […]
no intention from my side at all, I will put here whatever is appropriate, this is most probably just a copy-paste/ignorance error


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@35
PS6, Line 35: enum mncc_fsm_event {
> I think this file definitely needs quite a lot more comments. […]
ack, got increasingly impatient towards finishing this patch... I appreciate the review to highlight these places.


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@105
PS6, Line 105: struct mncc {
> this one unfortunately not.  "MNCC" is mobile netowkr call control. […]
came from removing "fsm" from "mncc_fsm" ... it is the FSM instance's "priv pointer struct".

Also the underlying intent is to pull the older MNCC implementation in gsm_04_08_cc.c away from the intermingled GSM CC and replace also the non-inter-MSC MNCC handling with this FSM. Then we would have only a single MNCC handler implementation, and this would be *the* definitive MNCC state for each and any call leg.

Should I change it? Suggestions? mncc_fsm_priv? mncc_fsm_state? mncc_state?
Usually I called things lchan_fsm and struct gsm_lchan, msc_a_fsm and struct msc_a, ran_peer_fsm and struct ran_peer;
But I notice now, also I have msc_ho_fsm and msc_ho_state, in this patch. Any preferences?


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@125
PS6, Line 125: 	int parent_event_call_setup_complete;
> the name seems to hint a true/false property?
it's an event to be dispatched when it is complete,
and indeed a negative value indicates that nothing should be dispatched.
Event numbers must range 0 <= event < 32.

Documented at mncc_alloc(), could also use a comment here.


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_a.h
File include/osmocom/msc/msc_a.h:

https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_a.h@75
PS6, Line 75: 		} ciphering;
> neither AKA nor IMEISV retrieval are strictly speaking part of ciphering.  Just saying. […]
Once the Classmark Update is finished, this is the state that allows continuing whatever code path triggered the Classmark Request.

So far there is only one code path that triggers a Classmark Request: ciphering, but the idea is that other callers could be added later.

The MSC is asked by the VLR to do Ciphering.
The MSC notices that it doesn't have sufficient Ciphering algo capabilities information.
Hence the Ciphering code path of the MSC triggers a Classmark Update.
This is state needed for encoding the Ciphering Mode Command message,
which the VLR requested when asking for the Ciphering originally.
The idea is to not involve the VLR in the Classmark Update, by storing the VLR's request.

--> should become a comment


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_a.h@80
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 st […]
good idea


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_roles.h
File include/osmocom/msc/msc_roles.h:

https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_roles.h@379
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 […]
hmm, indeed, didn't see that... will consider it


https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/nas.h
File include/osmocom/msc/nas.h:

https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/nas.h@111
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. […]
geran_encr consists of the Key and an Algorithm ID. The Key has its own IE, and the Algorithm is only used for when one specific algo has already been chosen.

A Handover Request sends separate IEs for
- the key
- permitted algorithms, composed from the a5_encryption_mask,
- actually chosen Algo ("Serving")

I guess the new BSS could choose a different ciphering algorithm(?) (but using the same key).

In this patch we always send both the "Chosen Encryption Algorithm (Serving)" (which is optional in the specs) and the "Encryption Information" (mandatory), but technically one could want to omit the Algorithm IE(??) -- doesn't really make sense, I agree, but I tried to be least restrictive.


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/call_leg.c
File src/libmsc/call_leg.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/call_leg.c@64
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 automaticall […]
agree.

The net is not required for registration; we could even define one global reference to the gsm_network and use it here.
This is only "shadowing" the msc_main.c allocated gsm_network struct, to make it more explicit where the gsm_network is coming from == cosmetics only.

(gsm_network simply is our global kitchen sink, and this can only be feeble / cute attempts to make it look slightly less like a magic global singleton that everyone accesses chaotically.)


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/e_link.c
File src/libmsc/e_link.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/e_link.c@380
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. […]
wha, this is a leftover that is not used. It was a useless premature optimization which didn't work out anyway, because it becomes impossible to transparently send/receive messages via an e-link with this. Should have been dropped, thanks for noticing.


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_08.c@248
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
indeed, copy paste error


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_80.c
File src/libmsc/gsm_04_80.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_80.c@39
PS6, Line 39: subscriber
> does msc_a really refference a subscriber?
yes; the MSC-A is the role that handles all subscriber management, and could be seen as "the subscriber".
The MSC-I/T roles are merely the active/transitional RAN connections.
The msub is the combination of MSC-A,-I and -T, which might be distributed across several MSC instances.

So it is technically correct to call msc_a the active subscriber.
But I also agree that the comment could say "The active subscriber's MSC-A role"


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsup_client_mux.c
File src/libmsc/gsup_client_mux.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsup_client_mux.c@37
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 could understand the comment if it was an Error or Notice, but a Debug is fine, isn't it?


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_ho.c
File src/libmsc/msc_ho.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_ho.c@138
PS6, Line 138: 	}
             : 
             : 	if (success) {
> else ?
(I think this happened because there was a 'return' above in an earlier version)


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_vty.c@512
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). […]
My only problem is that we apparently have no precedence of configuring an MSISDN as anything else than just a plain MSISDN ... ?
I would invent them without using them ...

We could also keep this for a future patch, an I think I already put the 'range' keyword in there to allow easily adding more variants, or modifiers like

  handover-number ton TON_VAL
  handover-number npi NPI_VAL
  handover-number range ...


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_vty.c@522
PS6, Line 522: FIXME leading zeros?? 
> Indeded, it probably makes sense not to treat this as unsigned integers but as some kind of "digit p […]
I picked what seemed most trivial to implement, but also:

what if you want to assign handover numbers to a range not corresponding to decimal digits? Like

  handover range 12323 12342

If we enforce always using entire digits, then the above case could only utilize 12330..12339:

  handover digits 1233x

So I thought it would be better to indicate an explicit start and end.

But rethinking it now, I agree the 0012345xxx format is better and can even be extended to support partial digits:

  handover number digits 0012345xxx [ range 123 417 ]

where the range is overlayed to replace the 'x' of the digits mask, and if the range is omitted uses 0..MAX.
And we don't need to implement the 'range' part yet.

I have no really strong opinion.

Unsure: doesn't the TON also affect the prefix? e.g. 0049 vs. +49?
I'm not familiar with the subject...


https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c
File src/libmsc/nas_iu.c:

https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@173
PS6, Line 173: 	RANAP_IE_t *ranap_ie;
> don't we get warnings here? 'ies' is const but the stack variables here not?
I think the IEs_t is a const "parent struct" that has pointers to (non-const) dynamic allocations (which is a direct consequence and drawback of using the same structures for encoding and decoding in combination with dynamic allocation).



-- 
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: 6
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-Comment-Date: Sun, 14 Apr 2019 17:06:09 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190414/85752a61/attachment.html>


More information about the gerrit-log mailing list