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/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgNeels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11642 )
Change subject: Add SGs Interface
......................................................................
Patch Set 29: Code-Review-1
(35 comments)
Haven't managed to read everything yet. so far...
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h
File include/osmocom/msc/ran_conn.h:
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h@216
PS29, Line 216: RAN_CONN_USE_SGs_SERVICE
> Missing comma.
(yes, it is nicer to keep the last comma, so later additions are single-line patches)
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_iface.h
File include/osmocom/msc/sgs_iface.h:
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_iface.h@29
PS29, Line 29: char *sockname;
Here, I guess talloc ownership is clear and straightforward, so it would be ok to keep this as-is,
but generally I'm in favor of char[123] storage (see osmo_sock_get_name_buf()).
(I now see it as a mistake that I implemented osmo_sock_get_name() as a talloc string.)
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h
File include/osmocom/msc/sgs_server.h:
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h@1
PS29, Line 1: /* (C 2018 by Harald Welte <laforge at gnumonks.org>
closing brace (C), and I guess it should contain 2019
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h@23
PS29, Line 23: #define DEFAULT_VLR_NAME "vlr.example.net"
Maybe this name could be "*_SGS_SERVER_*"?
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_vty.h
File include/osmocom/msc/sgs_vty.h:
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_vty.h@1
PS29, Line 1: /* (C 2018 by Harald Welte <laforge at gnumonks.org>
I think normally we don't have (C) in .h files?
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h
File include/osmocom/msc/vlr.h:
https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h@114
PS29, Line 114: struct vlr_subscr;
> Why do we need this? I don't see any references to this struct.
the struct is defined 7 lines further down; indeed drop this
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c@453
PS29, Line 453: struct msgb *gsm48_create_mm_info(struct gsm_network *net)
In principle, we put the bare msg composition stuff in libosmocore.
(It would be some effort to pull this apart from the osmo-msc local code base, though... so I'm not going to block on it)
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c
File src/libmsc/gsm_subscriber.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@119
PS29, Line 119: Note
> I think this note should precede the function definition.
Agree, return val info should go to the "API doc" string.
In general, I'd prefer to not start comments with "Note:".
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@130
PS29, Line 130: case OSMO_RAT_EUTRAN_SGS:
meta question:
IIUC on the EUTRAN, the subscriber does not actually send a LU request to the 2G MSC.
When a call gets established or SMS handling takes place, the SGs sends something, and then the MS is implicitly set to "attached".
So if an MS is attached to some LTE service, and if now the 2G MSC wants to send it an SMS or call it, that can only work if the MS has before used some 2G fallback service?
Shouldn't all paging already be managed by the LTE side, and the 2G MSC isn't even responsible for paging, and only take over from the LTE once instructed over SGs?
So, it doesn't make sense to me that an LTE subscriber is even listed as 'attached' in our VLR beyond an ongoing SGs transaction. Also, we will not get periodic location updates, and the VLR will remove the subscriber as soon as that period has passed; after that, we will again reject all paging.
How does all this work?
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@151
PS29, Line 151: * \param serv_ind sgsap service indicator (in case SGs interface is used to page)
end with dot
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/msc_vty.c@705
PS29, Line 705: /* SGs related */
maybe
if (vsub->sgs_fsm && vsub->sgs_fsm->state != SGS_UE_ST_NULL) {
...
?
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c
File src/libmsc/sgs_iface.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@3
PS29, Line 3: /* (C 2018 by Harald Welte <laforge at gnumonks.org>
) 19
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@98
PS29, Line 98: return vsub;
This is a bad thing to do in general.
You should keep the use count until done with the vlr subscr.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@110
PS29, Line 110: LOGSGC(sgc, LOGL_ERROR, "(sub %s) Cannot allocate connection, subscriber not found in HLR!\n", imsi);
this is not the HLR ... the vlr_subscr_find_by_imsi() does not contact the HLR.
I am wondering, IUUC the SGs implicitly sets a subscriber as attached. But we might want to first verify that our HLR even knows that IMSI, i.e. actually run a Location Updating Request procedure over GSUP? Later code does look like the SGs is doing that, but this here looks like it doesn't.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@121
PS29, Line 121: conn->vsub = vlr_subscr_get(vsub);
instead of another get() here, keep the get() use count from vlr_subscr_find_by_imsi().
In above 'if (!conn)' err handling do a vlr_subscr_put().
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@277
PS29, Line 277: vsub = vlr_subscr_by_imsi(gsm_network->vlr, imsi);
In another VTY related patch I did argue that what you're doing here is safe.
But in that patch I explicitly wanted the subscriber's use count to be printed without the temporary get().
Here please avoid handling a vsub outside of a get()..put() scope.
I know it gets a bit more cumbersome, but for code maintainability it's worth it.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@329
PS29, Line 329: if (imsi) {
this is not so nice, since you're obtaining log context info, even though you don't know whether something gets logged yet. We usually omit all string composition when the logging category is switched off, within the LOGFOO() macros.
The pattern I recently use a lot is: at the beginning, when allocating an FSM instance, place all relevant context information into the FSM instance id; then use LOGPFSML everywhere. That saves the effort of obtaining the log context over and over. Is something similar possible here?
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@348
PS29, Line 348: else
you are *always* logging an error now. I guess the initial 'if (sgsap_iei < 0)' should have a { .. } enclosing all the other error logging? Also maybe that {..} should end in 'return -EINVAL'?
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@354
PS29, Line 354: return 0;
[after reading other code] ...the "status" is intended to always indicate an error on the SGs? Then above would even be ok.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@414
PS29, Line 414: msg = gsm29118_create_mm_info_req(vsub->imsi, msg_mm_info->data + 2, msg_mm_info->len - 2);
(when we're sending MM Info data, is that really a "Request"?)
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@454
PS29, Line 454: * we are supposed to start a search procedure as defined in 3GPP TS 23.018 */
Of course our VLR does not even have a VLR subscriber stored if it is not attached. As in my other comment, I doubt that this is useful as it is now.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@851
PS29, Line 851: LOGSGC(sgc, LOGL_NOTICE, "SGsAP Message %s parsing error\n", sgsap_msg_type_name(msg_type));
LOGL_ERROR? Also below?
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@865
PS29, Line 865: msgb_free(msg);
you are often repeating the pattern "create_status, tx, free". That would qualify for a little static function to avoid code dup. You could even create something like an 'sgs_fail()' function or macro that also takes a log string, hence do all of error-logging and status tx in one function/macro invocation.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@871
PS29, Line 871: /* FIXME: Is there a well defined minimum length (MCC=3 + MNC=2 + MSIN=1 => 6?) */
GSM23003_IMSI_MIN_DIGITS
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@872
PS29, Line 872: if (imsi[0] == '\0' || strlen(imsi) <= 6) {
(strlen() is enough, it implies a "[0] == '\0' check")
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@984
PS29, Line 984: sgs_tx(sgc, resp);
others free() after sgs_tx(). One or the other is wrong.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@987
PS29, Line 987: OSMO_ASSERT(false);
...
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1083
PS29, Line 1083: LOGMME(mme, LOGL_ERROR, "Ts11 expired more than Ns11 times, giving up\n");
(would be nice to also log the Ns11 value)
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_server.c
File src/libmsc/sgs_server.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_server.c@1
PS29, Line 1: /* (C 2018 by Harald Welte <laforge at gnumonks.org>
) 19
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c
File src/libmsc/sgs_vty.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@1
PS29, Line 1: /* (C 2018 by Harald Welte <laforge at gnumonks.org>
) 19
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@99
PS29, Line 99: "timer (Ts5|TS6-2|Ts7|Ts11|Ts14|Ts15) <1-120>",
have you tested that the keywords are allowed to contain capital letters?
Normally for VTY all-caps means that it is a free-form arg.
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@123
PS29, Line 123: "counter (Ns7|Ns11) <0-255>",
same capital chars: tested?
https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@142
PS29, Line 142: "show sgs-connections", SHOW_STR
(prefer line break before SHOW_STR)
https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/Makefile.am
File src/libvlr/Makefile.am:
https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/Makefile.am@11
PS29, Line 11: $(LIBOSMOGSM_CFLAGS) \
interesting, why is this needed?
So far the idea of the VLR is to be separate from GSM specifics. It calls vlr->ops.foo callback functions to request actions, and it receives readily parsed function arguments. So far all message encoding and decoding happens in the MSC or the GSUP client, not in libvlr.
https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr.c
File src/libvlr/vlr.c:
https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr.c@838
PS29, Line 838: sgs_lu_in_progress = true;
this again looks like you are indeed invoking a Location Updating towards the HLR? Earlier the patch looked like that was not the case. Which is it / should it be?
--
To view, visit https://gerrit.osmocom.org/11642
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: I73359925fc1ca72b33a1466e6ac41307f2f0b11d
Gerrit-Change-Number: 11642
Gerrit-PatchSet: 29
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Stefan Sperling <stsp at stsp.name>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 16:30:43 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190108/01e6d100/attachment.htm>