Attention is currently required from: lynxis lazus.
12 comments:
Patchset:
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:
Patch Set #6, 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.
Patch Set #6, 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?
Patch Set #6, 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?
Patch Set #6, 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:
Patch Set #6, 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:
Patch Set #6, Line 923: // CKSN
let's stick to C style comments, laso below.
Patch Set #6, 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?
Patch Set #6, Line 1051: //GTP_LOGPKG(LOGL_ERROR, peer, pack, len,
looks like left-over / cleanup needed?
Patch Set #6, Line 1119: // resp_it++;
likewise, leftover/cleanup/bug?
Patch Set #6, 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.
Patch Set #6, Line 1328: quintlets
quintuplets?
To view, visit change 39563. To unsubscribe, or for help writing mail filters, visit settings.