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 <lyn...@fe80.eu> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: lynxis lazus <lyn...@fe80.eu> Gerrit-Comment-Date: Wed, 23 May 2018 16:19:48 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
