Attention is currently required from: lynxis lazus.

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

Change subject: Add Gn support to allow MME->SGSN, SGSN->MME cell reselection ......................................................................

Patch Set 6:

(12 comments)

Patchset:

PS6: overall my gut feeling is that there's way too much open-coded pointer arithmetic and manual buffer/length/index handling for my taste. If we cannot use msgb for some reason (it has stuff like msgb_may_pull, msgb_tailroom, ...), then maybe there's some other/better helpers we can develop for this use case?

File src/sgsn/gprs_gmm.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/b83b3465_d5fbbdee?usp... : PS6, Line 1488: static int get_guti(struct msgb *msg, struct gprs_gmm_ra_upd_req *req, uint32_t ptmsi, struct osmo_guti *guti, uint16_t *nas_token) I would usually assume that the first argument is the non-const output, followed by const arguments for input-only data. Here, I guess 'req' can be const. But then it seems nas_token is also an output argument.. so the function not only gets guti but also nas_token? And msg is not used at all?

I guess this needs some thought about naming, unused arguments and const input data.

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/c1721823_4e2bd641?usp... : PS6, Line 1628: LOGP(DGPRS, LOGL_ERROR, "Can't decode guti\n"); LOGIUP at least, so we get some context? Possibly even the entire input data so that we have a chane of figuring out why this happens if we ever see the error?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/36d37e70_19e60e6c?usp... : PS6, Line 1635: LOGP(DGPRS, LOGL_ERROR, "No MME found for Gummei %s", osmo_gummei_name(&guti.gummei)); Existing code above uses LOGIUP, so for consstency also add that?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/36d1fbf9_323ca7f7?usp... : PS6, Line 2625: /* FIXME: reject with impl. */ maybe at least log an error if this ever happens and we know we should be doing something?

File src/sgsn/gprs_rau_fsm.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/e459dd21_61b24d7d?usp... : PS6, Line 211: /* FIXME */ this is the fourth fixme in less than 20 lines of code... are we sure this is ready yet?

File src/sgsn/sgsn_libgtp.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/c381fd58_13bcd666?usp... : PS6, Line 923: // CKSN let's stick to C style comments, laso below.

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/4009c195_63479ac3?usp... : PS6, Line 1002: MEMCPY_CHK(ptr, &mmctx->drx_parms, sizeof(mmctx->drx_parms)); all of this custom working with a pointer and bounds checking makes me wonder why the code is not using a msgb here? doesn't have to be the actual overall signaling message msgb, but just a local buffer?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/31ff63cc_ff275571?usp... : PS6, Line 1051: //GTP_LOGPKG(LOGL_ERROR, peer, pack, len, looks like left-over / cleanup needed?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/dc3030a8_5b7d50e7?usp... : PS6, Line 1119: // resp_it++; likewise, leftover/cleanup/bug?

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/0e47ab01_41393e1f?usp... : PS6, Line 1151: static int validate_quintlets(uint8_t *buf, unsigned int buf_len) are there really quintlets in 3GPP somewhere? I'm familiar with quintets or quintuplets so far.

https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/d7c70b80_64fb3cc6?usp... : PS6, Line 1328: quintlets quintuplets?