Change in osmo-msc[master]: Add SGs Interface

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.org
Thu Jan 17 18:27:20 UTC 2019


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

Change subject: Add SGs Interface
......................................................................


Patch Set 30: Code-Review-1

(42 comments)

Good work so far; I did still find a lot of remaining problems, but we're well on our way to get this merged.

(Some of the remarks are optional to fix, skip those where you are sure and prefer to leave as is)

https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_iface.h
File include/osmocom/msc/sgs_iface.h:

https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_iface.h@42
PS30, Line 42: 	/* global list of MME contexts */
this is the *entry* in that list, right?


https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h
File include/osmocom/msc/sgs_server.h:

https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@4
PS30, Line 4: #define DEFAULT_VLR_NAME "*_SGS_SERVER_*"
I meant to name the variables DEFAULT_SGS_SERVER_IP and _NAME, not the enclosed URL.


https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@18
PS30, Line 18: 		char *local_addr;
(char array?)


https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@21
PS30, Line 21: 		char *vlr_name;
... here I'd prefer char arrays.
(If you have reasons against it then keep it this way though.)


https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/vlr.h
File include/osmocom/msc/vlr.h:

https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/vlr.h@349
PS30, Line 349: 					      const char *imsi);
If an IMSI is present, it is already part of the vlr_subscr_name().
Also, since recently, vlr_susbcr_name() includes all available info on a subscriber, including IMSI, MSISDN and TMSI at the same time. (I think we shouldn't continue on the weird path I started with vlr_subscr_msisdn_or_name().)


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: 	conn->vsub->classmark.classmark1 = lu->classmark1;
> I think it makes sense to pull this apart, we already have that for BSSMAP and for SGs as well but I […]
ack


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_04_08.c@473
PS30, Line 473:  * See also GSM 04.08, chapter 9.2.15a */
(I guess preferably these days include the more specific 3GPP TS 24.008 or 44.008 ... nr; maybe next time.)


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c
File src/libmsc/gsm_09_11.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c@347
PS8, Line 347: RV_IND_SMS);
> I am not sure, I have now made it look like other function calls in the file. […]
Vadim, aligning function arguments with the opening brace is a common code formatting style, also default of vim's cindent. I've used this style ever since I started in Osmocom (and also in other code bases before that).

I would gladly adopt the single-tab indent if I could only figure out cindent config to do so.
Do you use vim? Do you happen to know a cindent rule set to achieve single tab?

I am definitely not going to manually edit all function arguments indenting and fight against cindent, then I'd rather just keep cindent's align-with-opening-brace; I am using automatic indenting extensively, doing it manually would multiply the time i need to write code.

Consistency wise, I've seen both single-tab and align-with-brace indenting in the Osmocom code base, though align-with-brace seems to be more common.

(Although, for function arguments, two tabs would actually make more sense, especially for function definitions where the following body also has one tab indent.)


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: SCCP
> This is a static function, so its not part of the API and there shouldn't be a doc-string.
just because it is a static function doesn't mean you should drop common commenting practices :P
We're not as strict with static functions, but if you are documenting the function's return code, then put it in a comment above the function definition.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@130
PS29, Line 130: 		return sgs_iface_tx_paging(vsub, serv_ind);
> When the MME performs an LU on the SGs the VLR knows that the subscriber exists and in case of pagin […]
ack


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_subscriber.c
File src/libmsc/gsm_subscriber.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/gsm_subscriber.c@120
PS30, Line 120: 
(unrelated ws)


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@354
PS29, Line 354:  * reject, depending on the outcome of the location update. */
> Thats correct. […]
then all of this is fine, including the LOGL_ERROR.

(I am sometimes a bit unsure whether services being rejected should qualify for LOGL_ERROR or LOGL_NOTICE, but so far I've always decided for LOGL_ERROR in the end)


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@414
PS29, Line 414: 
> Yes, at least SGs calls it a "Request", see 3GPP TS 29.118 8.12. […]
ack; same on the E-interface, where DTAP is forwarded both ways as "Request"s, regardless whether they are a response to earlier DTAP or not.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@772
PS29, Line 772: 	/* Allocate subscriber connection */
> You mean when a subscriber has just created a conn on 2G/3G, then looses reception and the UE does a […]
for starters, this code path could check that via_ran == OSMO_RAT_EUTRAN_SGS and error out otherwise.
In the long run it might make more sense to discard the other conn, to not have to wait a periodic lu expiry before SGs attach becomes possible?


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@984
PS29, Line 984: 		break;
> The checks above abort the function early. […]
ah, of course, thx


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1212
PS29, Line 1212: 	 *  the VLR subscriber usage to be balanced. */
> This is the only FSM we need to register here. […]
last time i grepped it wasn't there (?) but it's definitely there now. ack.


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c
File src/libmsc/sgs_iface.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@54
PS30, Line 54: /* See also comment in a_iface.c */
which one, "The pointer is set once when calling a_init()"? :)


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@99
PS30, Line 99: 	conn->vsub = vsub;	
This should be vlr_subscr_get(vsub), right??

(whitespace error)


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@100
PS30, Line 100: 	conn->vsub->cs.attached_via_ran = conn->via_ran;
here (after assigning vsub and via_ran) please call
  ran_conn_update_id(conn)
for logging context.


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@103
PS30, Line 103: 	 * authenticated by the MME no authentication is requred */
"required"


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@117
PS30, Line 117: 	       vlr_subscr_msisdn_or_name(vsub));
would still prefer if the LOGxxx macros include the log ctx implicitly. Actually, I have recently added such to ran_conn, so:

IMO what you should do is, in ran_conn.c, in ran_conn_alloc add

  case OSMO_RAT_EUTRAN_SGS:
     conn->log_subsys = DSGS;

and then use LOG_RAN_CONN(conn, LOGL_DEBUG, "Allocated\n");

(the conn's FI should anyway already log that it is allocated, but use LOG_RAN_CONN() for all SGs logging; then you will always have the full subscriber and conn context added to the logging implicitly)

For places where there is no ran_conn available I'm not sure how to best solve this, but if possible also go for logging macros that include all applicable context info implicitly. (I think I've mentioned this in an earlier review; maybe you decided it is not possible?)


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@132
PS30, Line 132: 	       vlr_subscr_msisdn_or_name(vsub));
LOG_RAN_CONN()


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@245
PS30, Line 245: 		     vlr_subscr_msisdn_or_name(vsub), sgsap_msg_type_name(msg_type));
(rather just vlr_subscr_name(), same below)


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_server.c
File src/libmsc/sgs_server.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_server.c@187
PS30, Line 187: 	talloc_free(name);
suggest using osmo_sock_get_name2(), needs no talloc_free()


https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_vty.c
File src/libmsc/sgs_vty.c:

PS30: 
I would appreciate if you could add some .vty transcript test for the SGs VTY nodes, verifying that the new commands work as expected, show the expected docs and print back the expected 'show running-config'.


https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr.c
File src/libvlr/vlr.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr.c@125
PS30, Line 125: 					      const char *imsi)
If you want to keep this, the name more accurately would be 'vlr_subscr_imsi_or_msisdn_or_name()' ... but again rather just use vlr_subscr_name(), as we now also do in most FSM instance IDs and log contexts


https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c
File src/libvlr/vlr_sgs.c:

https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c@1
PS3, Line 1: /* (C) 2018-2019 by sysmocom s.f.m.c. GmbH
-2019?

feel free to grep the other files yourself...

(BTW, I am often thinking we should automate the copyright year indications somehow, by which years contain changes to the file...)


https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c@91
PS3, Line 91: 	vsub->sgs.paging_cb = paging_cb;
> I removed this one, but there are others. I am using them as orientation while testing things. […]
confirmed: I grepped the latest patch set for '\<printf' and found no hits.


https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c
File src/libvlr/vlr_sgs.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@215
PS30, Line 215: 	vlr_subscr_put(vsub);
throughout this file: do the vlr_subscr_put() *below* all uses! That's the whole idea of ref counting.


https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@223
PS30, Line 223: 	osmo_fsm_inst_dispatch(vsub->sgs_fsm, SGS_UE_E_RX_PAGING_FAILURE, &cause);
you are still using it here.

Same in other functions in this file, not marking each one now.


https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@308
PS30, Line 308: 	vsub->sgs.paging_serv_ind = serv_ind;
I'm getting the impression that you should do another vlr_subscr_get() to increase the vlr_subscr's use count while paging, and decrease when paging is done. Have you given this consideration yet?

For example, when above Ts5_timeout_cb() gets invoked, you want to be sure that the vsub has not been deallocated yet, even if the subscriber has somehow detached from the SGs (like an IMSI detach or an SGs connection reset?)


https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.h
File src/libvlr/vlr_sgs_fsm.h:

https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.h@35
PS30, Line 35: 	SGS_UE_E_RX_SGSAP_UE_UNRECHABLE,
UNREACHABLE with A


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c
File src/libvlr/vlr_sgs_fsm.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@227
PS5, Line 227: 
(here and below would prefer N-tabs indent instead of align-with-opening-brace; curly braces are definitely no candidates for align-with-brace)


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@231
PS5, Line 231: 	struct vlr_subscr *vsub = fi->priv;
one more indent for multi-line member assignment


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@238
PS5, Line 238: 		/* See 5.4.3 and 5.5.3 */
more indent


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@250
PS5, Line 250: 	default:
more indent


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@252
PS5, Line 252: 		break;
more indent


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@262
PS5, Line 262: 		/* Failed TMSI reallocation procedure, deallocate all TMSI
rather indent with two tabs (and how did you end up with the weird line breaks)


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@293
PS5, Line 293: 		    S(SGS_UE_E_RX_SGSAP_UE_UNRECHABLE) | S(SGS_UE_E_RX_PAGING_FAILURE) | S(SGS_UE_E_RX_ALERT_FAILURE) |
drop?


https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.c
File src/libvlr/vlr_sgs_fsm.c:

https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.c@61
PS30, Line 61: 		LOGPFSML(fi, LOGL_ERROR, "(sub %s) VLR LU tmsi allocation failed\n", vlr_subscr_msisdn_or_name(vsub));
use vlr_subscr_name(), or rather have that context in the fi->id already


https://gerrit.osmocom.org/#/c/11642/30/tests/msc_vlr/stubs.h
File tests/msc_vlr/stubs.h:

https://gerrit.osmocom.org/#/c/11642/30/tests/msc_vlr/stubs.h@5
PS30, Line 5:  * Author: Neels Hofmeyr <nhofmeyr at sysmocom.de>
that's me, though.
are you copying another stubs.h that we could just re-use instead of copying?


https://gerrit.osmocom.org/#/c/11642/30/tests/sms_queue/sms_queue_test.c
File tests/sms_queue/sms_queue_test.c:

https://gerrit.osmocom.org/#/c/11642/30/tests/sms_queue/sms_queue_test.c@240
PS30, Line 240: 
are these the same as stubs.h?



-- 
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: 30
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-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: Stefan Sperling <stsp at stsp.name>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Thu, 17 Jan 2019 18:27:20 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190117/d812192f/attachment.htm>


More information about the gerrit-log mailing list