<p><a href="https://gerrit.osmocom.org/13137">View Change</a></p><p>59 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/gsup_client_mux.h">File include/osmocom/msc/gsup_client_mux.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/gsup_client_mux.h@13">Patch Set #6, Line 13:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">could use some comment about what a "GSUP Client Mux" actually is all about.</p></li></ul></li><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 style="white-space: pre-wrap; word-wrap: break-word;">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?</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 style="white-space: pre-wrap; word-wrap: break-word;">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?</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 style="white-space: pre-wrap; word-wrap: break-word;">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.</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@64">Patch Set #6, Line 64:</a> <code style="font-family:monospace,monospace">struct mncc_outgoing_call_req {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the struct has a self-explanatory name, good.</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 style="white-space: pre-wrap; word-wrap: break-word;">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.</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 style="white-space: pre-wrap; word-wrap: break-word;">the name seems to hint a true/false property?</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@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace">GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same AGPLv3+ 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@75">Patch Set #6, Line 75:</a> <code style="font-family:monospace,monospace"> } ciphering;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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 style="white-space: pre-wrap; word-wrap: break-word;">not imporant, but one could have an OSMO_ASSERT on the 'offsetof() == 0' somewhere during program startup.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_ho.h">File include/osmocom/msc/msc_ho.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_ho.h@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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 style="white-space: pre-wrap; word-wrap: break-word;">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.</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 style="white-space: pre-wrap; word-wrap: break-word;">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?</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 style="white-space: pre-wrap; word-wrap: break-word;">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</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/cell_id_list.c">File src/libmsc/cell_id_list.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/cell_id_list.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/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 style="white-space: pre-wrap; word-wrap: break-word;">*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.</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 style="white-space: pre-wrap; word-wrap: break-word;">comment and function name don't seem to agree on what this function is doing</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 style="white-space: pre-wrap; word-wrap: break-word;">does msc_a really refference a subscriber?</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 style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/mncc_fsm.c">File src/libmsc/mncc_fsm.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/mncc_fsm.c@10">Patch Set #6, Line 10:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a.c">File src/libmsc/msc_a.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_a.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">licnese</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/src/libmsc/msc_a.c@983">Patch Set #6, Line 983:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_fsm_register(&msc_a_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">can be moved to __Attribute__((constructor))</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a_remote.c">File src/libmsc/msc_a_remote.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_a_remote.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msc_a_remote.c@364">Patch Set #6, Line 364:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_fsm_register(&msc_a_remote_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">can be moved to __attribute__((constructor))</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@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msc_ho.c@65">Patch Set #6, Line 65:</a> <code style="font-family:monospace,monospace"> osmo_fsm_register(&msc_ho_fsm);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">can be moved to __attribute__((constructor))</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/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 style="white-space: pre-wrap; word-wrap: break-word;">else ?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i.c">File src/libmsc/msc_i.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_i.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msc_i.c@310">Patch Set #6, Line 310:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">__attribute__((constructor)) avoids having to explicitly call this function and can make it static.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i_remote.c">File src/libmsc/msc_i_remote.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_i_remote.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msc_i_remote.c@225">Patch Set #6, Line 225:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_fsm_register(&msc_i_remote_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">__attribute__((constructor))</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t.c">File src/libmsc/msc_t.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_t.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msc_t.c@872">Patch Set #6, Line 872:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">constructor</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t_remote.c">File src/libmsc/msc_t_remote.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_t_remote.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msc_t_remote.c@204">Patch Set #6, Line 204:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_fsm_register(&msc_t_remote_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">constructor</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 style="white-space: pre-wrap; word-wrap: break-word;">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?</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/src/libmsc/msc_vty.c@522">Patch Set #6, Line 522:</a> <code style="font-family:monospace,monospace">FIXME leading zeros?? </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msub.c">File src/libmsc/msub.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/msub.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/msub.c@152">Patch Set #6, Line 152:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_fsm_register(&msub_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">constructor</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas.c">File src/libmsc/nas.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.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_a.c">File src/libmsc/nas_a.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_a.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</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/src/libmsc/nas_iu.c@95">Patch Set #6, Line 95:</a> <code style="font-family:monospace,monospace">static void nas_iu_decode_l3(struct nas_dec *nas_iu_decode, RANAP_NAS_PDU_t *nas_pdu, const char *msg_name)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const</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/src/libmsc/nas_iu.c@114">Patch Set #6, Line 114:</a> <code style="font-family:monospace,monospace">static void nas_iu_decode_err(struct nas_dec *nas_iu_decode, RANAP_ErrorIndicationIEs_t *ies)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const</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/src/libmsc/nas_iu.c@123">Patch Set #6, Line 123:</a> <code style="font-family:monospace,monospace"> RANAP_RAB_SetupOrModifiedItemIEs_t *setup_ies)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">const</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/src/libmsc/nas_iu.c@125">Patch Set #6, Line 125:</a> <code style="font-family:monospace,monospace"> RANAP_RAB_SetupOrModifiedItem_t *item;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">these here then subsequently also all const</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/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 style="white-space: pre-wrap; word-wrap: break-word;">don't we get warnings here? 'ies' is const but the stack variables here not?</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/src/libmsc/nas_iu.c@256">Patch Set #6, Line 256:</a> <code style="font-family:monospace,monospace">static void nas_iu_decode_ranap_msg(void *_nas_dec, ranap_message *message)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ideally the entire input '*message' would be const here, to ensure all downstream users also are 'const'</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/src/libmsc/nas_iu.c@416">Patch Set #6, Line 416:</a> <code style="font-family:monospace,monospace">static void ranap_handle_cl(void *ctx, ranap_message *message)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe even here const?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/neighbor_ident.c">File src/libmsc/neighbor_ident.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/neighbor_ident.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> *</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no SPDX identifier</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/paging.c">File src/libmsc/paging.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/paging.c@9">Patch Set #6, Line 9:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">mo license header at all</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_infra.c">File src/libmsc/ran_infra.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/ran_infra.c@8">Patch Set #6, Line 8:</a> <code style="font-family:monospace,monospace"> * SPDX-License-Identifier: GPL-2.0+</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_peer.c">File src/libmsc/ran_peer.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/ran_peer.c@14">Patch Set #6, Line 14:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license header missing</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/src/libmsc/ran_peer.c@19">Patch Set #6, Line 19:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT( osmo_fsm_register(&ran_peer_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">constructor</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/rtp_stream.c">File src/libmsc/rtp_stream.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/rtp_stream.c@12">Patch Set #6, Line 12:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">license header missing</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/src/libmsc/rtp_stream.c@38">Patch Set #6, Line 38:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_fsm_register(&rtp_stream_fsm) == 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">constructor</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/sccp_ran.c">File src/libmsc/sccp_ran.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/sccp_ran.c@5">Patch Set #6, Line 5:</a> <code style="font-family:monospace,monospace"> *</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">mp SPDX identifier</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/6/src/osmo-msc/msc_main.c">File src/osmo-msc/msc_main.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/osmo-msc/msc_main.c@83">Patch Set #6, Line 83:</a> <code style="font-family:monospace,monospace">2016</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">at the very least that line here should be updated to 2016-2019 now with those significant changes.</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 15:29:15 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>