Attention is currently required from: pespin.
lynxis lazus 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:
(10 comments)
File include/osmocom/sgsn/debug.h:
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/eb74b128_1d55f32f?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?
because libvlr is using it. I improved the comment.
File src/sgsn/gprs_gmm.c:
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/a6505a92_bf14b998?usp... : PS16, Line 199: Remove whitespace into cosmetic commit
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/7852c6f0_d348ceb0?usp... : PS16, Line 1443: TODO: cosmetic
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/2989f426_d43e78f6?usp... : PS16, Line 742: /* FIXME: the VLR should check the auth and not here a second time! */
what about all these FIXMEs?
Because I noticed issues and problems with the code and might not fix it, because it gets more and more into a single commit. The rework is working much better than the current state.
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/a34c85e6_755ea298?usp... : PS16, Line 893: move into cosmetic commit
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/6fbbe15e_00b099b8?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...
Done
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/5f80e1b4_d3d0da84?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...
Done
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/61975f75_76773983?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...
not completely. but did so far as possible
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/b9267f65_d5c0bb8a?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 conten […]
But the libvlr is very connected to the GMM layer. If we move it, I would like to have the discussion in a separate commit.
https://gerrit.osmocom.org/c/osmo-sgsn/+/39562/comment/7e168356_449322d9?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 […]
I would like to keep it as it is atm. I'm happy to review the refactoring of this. But it is related to libvlr and