<p style="white-space: pre-wrap; word-wrap: break-word;">I agree with all license,const comments.<br>I actually neglected to do a final pass for those things.<br>Another patch set will follow.</p><p><a href="https://gerrit.osmocom.org/13137">View Change</a></p><p>18 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc.h">File include/osmocom/msc/mncc.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc.h@200">Patch Set #6, Line 200:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">union mncc_msg {<br>      uint32_t msg_type;<br>    struct gsm_mncc signal;<br>       struct gsm_mncc_hello hello;<br>  struct gsm_mncc_rtp rtp;<br>      struct gsm_mncc_bridge bridge;<br>};<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why do we have a union of just a few MNCC mesage types? I could understand a union of all the possib […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h">File include/osmocom/msc/mncc_fsm.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace">GPL-2.0+</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">osmo-msc in general is AGPLv3+, why is GPLv2+ sggested here? Is there a plan to move this to a share […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">no intention from my side at all, I will put here whatever is appropriate, this is most probably just a copy-paste/ignorance error</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@35">Patch Set #6, Line 35:</a> <code style="font-family:monospace,monospace">enum mncc_fsm_event {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this file definitely needs quite a lot more comments. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ack, got increasingly impatient towards finishing this patch... I appreciate the review to highlight these places.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@105">Patch Set #6, Line 105:</a> <code style="font-family:monospace,monospace">struct mncc {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this one unfortunately not.  "MNCC" is mobile netowkr call control. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">came from removing "fsm" from "mncc_fsm" ... it is the FSM instance's "priv pointer struct".</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Should I change it? Suggestions? mncc_fsm_priv? mncc_fsm_state? mncc_state?<br>Usually I called things lchan_fsm and struct gsm_lchan, msc_a_fsm and struct msc_a, ran_peer_fsm and struct ran_peer;<br>But I notice now, also I have msc_ho_fsm and msc_ho_state, in this patch. Any preferences?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@125">Patch Set #6, Line 125:</a> <code style="font-family:monospace,monospace"> int parent_event_call_setup_complete;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">the name seems to hint a true/false property?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">it's an event to be dispatched when it is complete,<br>and indeed a negative value indicates that nothing should be dispatched.<br>Event numbers must range 0 <= event < 32.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Documented at mncc_alloc(), could also use a comment here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_a.h">File include/osmocom/msc/msc_a.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_a.h@75">Patch Set #6, Line 75:</a> <code style="font-family:monospace,monospace">          } ciphering;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">neither AKA nor IMEISV retrieval are strictly speaking part of ciphering.  Just saying. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Once the Classmark Update is finished, this is the state that allows continuing whatever code path triggered the Classmark Request.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The MSC is asked by the VLR to do Ciphering.<br>The MSC notices that it doesn't have sufficient Ciphering algo capabilities information.<br>Hence the Ciphering code path of the MSC triggers a Classmark Update.<br>This is state needed for encoding the Ciphering Mode Command message,<br>which the VLR requested when asking for the Ciphering originally.<br>The idea is to not involve the VLR in the Classmark Update, by storing the VLR's request.</p><p style="white-space: pre-wrap; word-wrap: break-word;">--> should become a comment</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_a.h@80">Patch Set #6, Line 80:</a> <code style="font-family:monospace,monospace">   /* struct msc_role_common must remain at start */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">not imporant, but one could have an OSMO_ASSERT on the 'offsetof() == 0' somewhere during program st […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">good idea</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_roles.h">File include/osmocom/msc/msc_roles.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_roles.h@379">Patch Set #6, Line 379:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct an_apdu {<br>  /* accessNetworkProtocolId */<br> enum osmo_gsup_access_network_protocol an_proto;<br>      /* signalInfo */<br>      struct msgb *msg;<br>     /* If this AN-APDU is sent between MSCs, additional information from the E-interface messaging, like the<br>       * Handover Number, will placed/available here. Otherwise may be left NULL. */<br>        const struct osmo_gsup_message *e_info;<br>};<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">as this just adds two 'long' words to 'struct msgb' one could have implemented this in arguably more […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">hmm, indeed, didn't see that... will consider it</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/nas.h">File include/osmocom/msc/nas.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/nas.h@111">Patch Set #6, Line 111:</a> <code style="font-family:monospace,monospace">          struct geran_encr *chosen_encryption;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">the comment explains that "0 means no such IE was present".  But the member is a pointer. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A Handover Request sends separate IEs for</p><ul><li>the key</li><li>permitted algorithms, composed from the a5_encryption_mask,</li><li>actually chosen Algo ("Serving")</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">I guess the new BSS could choose a different ciphering algorithm(?) (but using the same key).</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/call_leg.c">File src/libmsc/call_leg.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/call_leg.c@64">Patch Set #6, Line 64:</a> <code style="font-family:monospace,monospace">      OSMO_ASSERT( osmo_fsm_register(&call_leg_fsm) == 0 );</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would typically try to move as much as possible (particularly fsm registrations) into automaticall […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">agree.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The net is not required for registration; we could even define one global reference to the gsm_network and use it here.<br>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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(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.)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/e_link.c">File src/libmsc/e_link.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/e_link.c@380">Patch Set #6, Line 380:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Point a struct msgb's data members directly at the data buffer of gsup_msg->an_apdu. This *is* a hack, and the msgb<br> * returned is a static struct msgb: it must *not* be freed, put in a queue or kept around after gsup_msg->an_apdu is<br> * gone. The returned msgb can *not* be passed down a RAN peer's SCCP user SAP. This can be useful to "peek" at the<br> * included data by passing to a nas_decode implementation in a code path that does not pass any PDU on and wants to<br> * avoid dynamic allocation.<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">*really* ugly and easy to abuse/introduce bugs. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_08.c">File src/libmsc/gsm_04_08.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_08.c@248">Patch Set #6, Line 248:</a> <code style="font-family:monospace,monospace">int compl_l3_msg_is_r99(const struct msgb *msg)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">comment and function name don't seem to agree on what this function is doing</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">indeed, copy paste error</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_80.c">File src/libmsc/gsm_04_80.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsm_04_80.c@39">Patch Set #6, Line 39:</a> <code style="font-family:monospace,monospace">subscriber</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">does msc_a really refference a subscriber?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">yes; the MSC-A is the role that handles all subscriber management, and could be seen as "the subscriber".<br>The MSC-I/T roles are merely the active/transitional RAN connections.<br>The msub is the combination of MSC-A,-I and -T, which might be distributed across several MSC instances.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So it is technically correct to call msc_a the active subscriber.<br>But I also agree that the comment could say "The active subscriber's MSC-A role"</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsup_client_mux.c">File src/libmsc/gsup_client_mux.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/gsup_client_mux.c@37">Patch Set #6, Line 37:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">      LOGP(DLGSUP, LOGL_DEBUG, "No explicit GSUP Message Class, trying to guess from message type %s\n",<br>       osmo_gsup_message_type_name(gsup_msg->message_type));<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">as pretty much all existing GSUP messages by any existing program don't have the message_class set,  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I could understand the comment if it was an Error or Notice, but a Debug is fine, isn't it?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_ho.c">File src/libmsc/msc_ho.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_ho.c@138">Patch Set #6, Line 138:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">      }<br><br>   if (success) {<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">else ?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I think this happened because there was a 'return' above in an earlier version)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_vty.c">File src/libmsc/msc_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_vty.c@512">Patch Set #6, Line 512:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_msc_handover_number_range, cfg_msc_handover_number_range_cmd,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">MSISDNs typically also have NPI/TON (numbering plan / type of number). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">My only problem is that we apparently have no precedence of configuring an MSISDN as anything else than just a plain MSISDN ... ?<br>I would invent them without using them ...</p><p style="white-space: pre-wrap; word-wrap: break-word;">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</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  handover-number ton TON_VAL<br>  handover-number npi NPI_VAL<br>  handover-number range ...</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_vty.c@522">Patch Set #6, Line 522:</a> <code style="font-family:monospace,monospace">FIXME leading zeros?? </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Indeded, it probably makes sense not to treat this as unsigned integers but as some kind of "digit p […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I picked what seemed most trivial to implement, but also:</p><p style="white-space: pre-wrap; word-wrap: break-word;">what if you want to assign handover numbers to a range not corresponding to decimal digits? Like</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  handover range 12323 12342</pre><p style="white-space: pre-wrap; word-wrap: break-word;">If we enforce always using entire digits, then the above case could only utilize 12330..12339:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  handover digits 1233x</pre><p style="white-space: pre-wrap; word-wrap: break-word;">So I thought it would be better to indicate an explicit start and end.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But rethinking it now, I agree the 0012345xxx format is better and can even be extended to support partial digits:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  handover number digits 0012345xxx [ range 123 417 ]</pre><p style="white-space: pre-wrap; word-wrap: break-word;">where the range is overlayed to replace the 'x' of the digits mask, and if the range is omitted uses 0..MAX.<br>And we don't need to implement the 'range' part yet.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have no really strong opinion.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Unsure: doesn't the TON also affect the prefix? e.g. 0049 vs. +49?<br>I'm not familiar with the subject...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c">File src/libmsc/nas_iu.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@173">Patch Set #6, Line 173:</a> <code style="font-family:monospace,monospace"> RANAP_IE_t *ranap_ie;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">don't we get warnings here? 'ies' is const but the stack variables here not?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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).</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13137">change 13137</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/13137"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I27e4988e0371808b512c757d2b52ada1615067bd </div>
<div style="display:none"> Gerrit-Change-Number: 13137 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 14 Apr 2019 17:06:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>