Change in osmo-sgsn[master]: gprs_gmm: introduce a GMM Attach Request FSM

[Harald Welte]

Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/9257 )

Change subject: gprs_gmm: introduce a GMM Attach Request FSM
......................................................................


Patch Set 1: Code-Review-1

(9 comments)

https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.h
File src/gprs/gprs_gmm_attach.h:

https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.h@7
PS1, Line 7: 	ST_INIT,
the states might warrant some more documentation/explanation in comments here


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.h@17
PS1, Line 17: 	E_ATTACH_REQ_RECV,
same goes for the events here.  The *_RECV and *_SENT are pretty lcear, but the others might not be?


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c
File src/gprs/gprs_gmm_attach.c:

https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@10
PS1, Line 10: extern const struct value_string gmm_attach_req_fsm_event_names[];
why declare the fsm and the event_names here?  Are they used anywhere in the filbe before they are defined further down?


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@71
PS1, Line 71: 	/* check if we received a identity response */
you don't have a switch statement or an OSMO_ASSERT on the "event" which mgiht be dangerous as the FSM definition might be edited in the future and this code implicitly assumes only a single event may arrive here?


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@89
PS1, Line 89: 	if (type == GSM_MI_TYPE_IMEI &&	!strlen(ctx->imsi)) {
no tab here


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@143
PS1, Line 143: 		/* FIXME!! */
this appears that UMTS AKA is not working with this new FSM?  Was it also broken before this FSM?  In that case: We cannot afford to introduce known regression.


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@155
PS1, Line 155: 	}
do we want to silently ignore any other events?  In other FSMs we explicitly OSMO_ASSERT() on any unexpecte events here.

Same applies to all other state handling functiosn.


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@175
PS1, Line 175: 		/* TODO: #ifdef ! PTMSI_ALLOC is not supported */
does this mean you're not supporting disabling or enabling of P-TMSI allocation?  I believe it should be enabled at all times these days, the #define was just a hack to disable it for debugging?


https://gerrit.osmocom.org/#/c/9257/1/src/gprs/gprs_gmm_attach.c@222
PS1, Line 222: 		.name = "Ask the hlr about the MS",
please use shorter more symbolic names, possibly even "ST_ASK_VLR" here.  This is used heavily in logging and also in the CTRL interface, not sure spaces are even permitted...



-- 
To view, visit https://gerrit.osmocom.org/9257
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58b9c17be9776a03bb2a5b21e99135cfefc8c912
Gerrit-Change-Number: 9257
Gerrit-PatchSet: 1
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: lynxis lazus <lynxis at fe80.eu>
Gerrit-Comment-Date: Wed, 23 May 2018 16:19:48 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180523/fba21d12/attachment.htm>