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?us… :
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?us… :
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?us… :
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?us… :
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?us… :
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?us… :
PS6, Line 923: // CKSN
let's stick to C style comments, laso below.
https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/4009c195_63479ac3?us… :
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?us… :
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?us… :
PS6, Line 1119: // resp_it++;
likewise, leftover/cleanup/bug?
https://gerrit.osmocom.org/c/osmo-sgsn/+/39563/comment/0e47ab01_41393e1f?us… :
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?us… :
PS6, Line 1328: quintlets
quintuplets?
--
To view, visit
https://gerrit.osmocom.org/c/osmo-sgsn/+/39563?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: Iffe83b31db2b6e6869fedf2351375184096cff6f
Gerrit-Change-Number: 39563
Gerrit-PatchSet: 6
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Mon, 10 Mar 2025 19:12:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No