Attention is currently required from: lynxis lazus.

pespin has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/39562?usp=email )

Change subject: Rework SGSN code to use new libvlr code ......................................................................

Patch Set 16:

(17 comments)

Patchset:

PS16: I'd really welcome if you spend some more effort splitting some stuff into separate preparatory patches.

File include/osmocom/sgsn/debug.h:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/e2b240b7_7f6d0970?usp... : PS16, Line 31: DSGS, /* only as placeholder, SGS isn't used by the SGSN */ I don't get it, if it's not used why adding it here?

File src/sgsn/gprs_gmm.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/b0f6fd43_b51a884d?usp... : PS16, Line 742: /* FIXME: the VLR should check the auth and not here a second time! */ what about all these FIXMEs?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/efcf62f3_0d31ddb2?usp... : PS16, Line 1092: rc = gprs_gmm_parse_att_req(msg, &req); Looks like addition of this parsing functin could be done easily in a preparation patch...

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/eff2ccf3_4bfcdd97?usp... : PS16, Line 1735: if (TLVP_PRES_LEN(&req.tlv, GSM48_IE_GMM_PDP_CTX_STATUS, 2)) { This line could have been easily a separate preparation commit...

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/0404648b_cc418135?usp... : PS16, Line 2106: rc = gsm48_rx_gmm_det_accept(mmctx, msg); The change to a function could have been done in a previous separate commit...

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/c1ab7dd1_89cc8b67?usp... : PS16, Line 2327: static int vlr_tx_auth_req_cb(void *ref, struct vlr_auth_tuple *at, bool send_autn) I think it makes sense to have all this VLR code in a separate file, there's already too much content in this GMM file imho.

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/02ca7342_dabbf67f?usp... : PS16, Line 2577: int gmm_vlr_init(struct sgsn_instance *sgi) This is not really related to a given GMM session, but global one, so it totally makes sense to move it to sgsn_vlr.c or similar.

File src/sgsn/gprs_gmm_fsm.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/136f0112_f3756223?usp... : PS16, Line 262: /* fixme: this state seems to be wrong. When in COMMON_PROC_INIT, we shouldn't go into deregistered. */ FIXME

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/76904a30_d2739051?usp... : PS16, Line 307: .allstate_event_mask = X(E_GMM_CLEANUP) | X(E_GMM_RAT_CHANGE) | X(E_GMM_SERVICE_ACCEPT) | X(E_GMM_SERVICE_REJECT), plese better move to one per line, it's much easier to read.

File src/sgsn/gprs_rau_fsm.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/47440108_851c5dd6?usp... : PS16, Line 76: static void gmm_rau_fsm_s_init(struct osmo_fsm_inst *fi, uint32_t event, void *data) Why is the file called "gprs_rau_fsm" vs all the types/functions being called "gmm_rau_fsm"? Please fix so they are aligned. I'm fine with renaming file to "gmm_rau_fsm" for instance.

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/53c67feb_e0954913?usp... : PS16, Line 303: .action = gmm_rau_fsm_s_init, I'd really welcome using at least "st" for state, not only "s".

File src/sgsn/sgsn_libgtp.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/c3e847e3_fdbb33d2?usp... : PS16, Line 534: return 0; /* Nothing to do */ This is the kind of fixes which can be done in a separate patch. I have seen plenty of whitespace fixing in several files which is totally unrelated to the already big changes you are doing.

File src/sgsn/sgsn_vty.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/3a6b6307_02fc5557?usp... : PS16, Line 806: /* FIXME: do we still have ACLs? */ what about this? afaiu in the commit message you said we removed it? Then all this needs to be deprecated.

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/e1bb1438_888727df?usp... : PS16, Line 985: /* FIXME: reset VLR */ what about this?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/f57dd287_13f38f32?usp... : PS16, Line 1003: /* FIXME list VLR or MMctx */ what about this?

File tests/sgsn/gprs_gb_parse.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/d3746455_0cc36b9b?usp... : PS16, Line 34: static int gprs_gb_parse_gmm_attach_req(uint8_t *data, size_t data_len, I wonder why this is all removed in the same commit where we switch to VLR. Is all this only used so we can support the ACL features? Then, can we please drop the ACL feature in one commit, then do the change?