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/.
Harald Welte gerrit-no-reply at lists.osmocom.orgHarald Welte 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:
(59 comments)
https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/gsup_client_mux.h
File include/osmocom/msc/gsup_client_mux.h:
https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/gsup_client_mux.h@13
PS6, Line 13:
could use some comment about what a "GSUP Client Mux" actually is all about.
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 possible options, but why only specificaly those?
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 shared library whihc might also be used from non-AGPLv3+ programs?
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. In general the states and events deserve some kind of description, as do the structs.
https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/mncc_fsm.h@64
PS6, Line 64: struct mncc_outgoing_call_req {
the struct has a self-explanatory name, good.
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. 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.
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?
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@8
PS6, Line 8: GPL-2.0+
same AGPLv3+ comment
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. This isn't bikeshedding, I just want to make sure we use as precise naming as possible to ease readability and avoid any misunderstandings.
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 startup.
https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_ho.h
File include/osmocom/msc/msc_ho.h:
https://gerrit.osmocom.org/#/c/13137/6/include/osmocom/msc/msc_ho.h@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
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 osmocom-style using the msgb->cb[] array and some accessor macros. No need to change it, I'm just sharing my thoughts.
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. So where's the semantic differenece to chosen_encryption = NULL?
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 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
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/cell_id_list.c
File src/libmsc/cell_id_list.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/cell_id_list.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
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@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
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. 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.
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
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?
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'm not sure we should log a DEBUG message for each of them. Maye in a few years time, but not today.
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/mncc_fsm.c
File src/libmsc/mncc_fsm.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/mncc_fsm.c@10
PS6, Line 10: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a.c
File src/libmsc/msc_a.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
licnese
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a.c@983
PS6, Line 983: OSMO_ASSERT(osmo_fsm_register(&msc_a_fsm) == 0);
can be moved to __Attribute__((constructor))
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a_remote.c
File src/libmsc/msc_a_remote.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a_remote.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_a_remote.c@364
PS6, Line 364: OSMO_ASSERT(osmo_fsm_register(&msc_a_remote_fsm) == 0);
can be moved to __attribute__((constructor))
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@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_ho.c@65
PS6, Line 65: osmo_fsm_register(&msc_ho_fsm);
can be moved to __attribute__((constructor))
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_ho.c@138
PS6, Line 138: }
:
: if (success) {
else ?
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i.c
File src/libmsc/msc_i.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i.c@310
PS6, Line 310: {
__attribute__((constructor)) avoids having to explicitly call this function and can make it static.
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i_remote.c
File src/libmsc/msc_i_remote.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i_remote.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_i_remote.c@225
PS6, Line 225: OSMO_ASSERT(osmo_fsm_register(&msc_i_remote_fsm) == 0);
__attribute__((constructor))
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t.c
File src/libmsc/msc_t.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t.c@872
PS6, Line 872: {
constructor
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t_remote.c
File src/libmsc/msc_t_remote.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t_remote.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msc_t_remote.c@204
PS6, Line 204: OSMO_ASSERT(osmo_fsm_register(&msc_t_remote_fsm) == 0);
constructor
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). probably makes sense to configure those explicitly somewhere and include them in the match?
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 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.
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msub.c
File src/libmsc/msub.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msub.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/msub.c@152
PS6, Line 152: OSMO_ASSERT(osmo_fsm_register(&msub_fsm) == 0);
constructor
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas.c
File src/libmsc/nas.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_a.c
File src/libmsc/nas_a.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_a.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
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@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@95
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)
const
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@114
PS6, Line 114: static void nas_iu_decode_err(struct nas_dec *nas_iu_decode, RANAP_ErrorIndicationIEs_t *ies)
const
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@123
PS6, Line 123: RANAP_RAB_SetupOrModifiedItemIEs_t *setup_ies)
const
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@125
PS6, Line 125: RANAP_RAB_SetupOrModifiedItem_t *item;
these here then subsequently also all const
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?
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@256
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'
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/nas_iu.c@416
PS6, Line 416: static void ranap_handle_cl(void *ctx, ranap_message *message)
maybe even here const?
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/neighbor_ident.c
File src/libmsc/neighbor_ident.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/neighbor_ident.c@8
PS6, Line 8: *
no SPDX identifier
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/paging.c
File src/libmsc/paging.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/paging.c@9
PS6, Line 9:
mo license header at all
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_infra.c
File src/libmsc/ran_infra.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_infra.c@8
PS6, Line 8: * SPDX-License-Identifier: GPL-2.0+
license
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_peer.c
File src/libmsc/ran_peer.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_peer.c@14
PS6, Line 14:
license header missing
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/ran_peer.c@19
PS6, Line 19: OSMO_ASSERT( osmo_fsm_register(&ran_peer_fsm) == 0);
constructor
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/rtp_stream.c
File src/libmsc/rtp_stream.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/rtp_stream.c@12
PS6, Line 12:
license header missing
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/rtp_stream.c@38
PS6, Line 38: OSMO_ASSERT(osmo_fsm_register(&rtp_stream_fsm) == 0);
constructor
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/sccp_ran.c
File src/libmsc/sccp_ran.c:
https://gerrit.osmocom.org/#/c/13137/6/src/libmsc/sccp_ran.c@5
PS6, Line 5: *
mp SPDX identifier
https://gerrit.osmocom.org/#/c/13137/6/src/osmo-msc/msc_main.c
File src/osmo-msc/msc_main.c:
https://gerrit.osmocom.org/#/c/13137/6/src/osmo-msc/msc_main.c@83
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 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 15:29:15 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190414/e3b0f1fa/attachment.htm>