Attention is currently required from: Hoernchen, laforge, fixeria.
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021 )
Change subject: gmm: Initial implementation of GPRS Attach
......................................................................
Patch Set 1:
(12 comments)
File include/osmocom/gprs/gmm/gmm_pdu.h:
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/3b3744f7_686e7dae
PS1, Line 16: tlv_definition
You should include `<osmocom/gsm/tlv.h>`
defining this struct.
Ack
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/a64c2952_60e95285
PS1, Line 32: uint8_t sres[4],
Why not a const pointer?
Ack
File libosmo-gprs-gmm.pc.in:
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/36094b3d_ec293ba4
PS1, Line 9: libosmo-gprs-llc
btw, why is this needed? because of header files?
Because gmm_prim uses the llc_prim for the LLGMM API, and it uses the struct
defintiions and functions to allocate the primitives.
That doesn't mean it forcefully needs the entire LLC stack in libosmo-gprs-llc to
function though, since the app is the one forwarding the primitives.
File src/gmm/gmm_pdu.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/25ad325b_0148c103
PS1, Line 31: struct gprs_gmm_ms_net_cap {
Run `struct_endianess.py`.
Ack
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/8477c5c3_f8aaa1e1
PS1, Line 142: 23
`GSM_MAC_BLOCK_LEN`?
yeah, but this is all
temporary to have some base upon to work with.
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/e9254ab7_4361010a
PS1, Line 147: rc = bitvec_unhex(&bv,
"171933432b37159ef98879cba28c6621e72688b198879c00");
Makes sense to add a TODO here, since you're
hard-coding the payload.
yeah, but this is all temporary to have some base upon to
work with.
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/9b2fa138_09e2e8b8
PS1, Line 172: msgb_put_u8(msg, sizeof(ms_net_cap_def));
Use `msgb_lv_put()`.
I'll check.
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/a1b3e56f_5585003d
PS1, Line 274: GSM_MI_TYPE_TMSI
`GSM_MI_TYPE_IMEI`!
Ack
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/de7981c6_5d1aa5b7
PS1, Line 312: (void)imeisv_requested;
What is this for?
IDK, I yet have to discover,
that's why it's TODO :D but it shows up in the specs in the messages.
if you mean the (void) thing, it's to avoid having the "empty if clause" or
alike warning. This is left here so that in the future we can quickly find out where to
use it once we read the related specs.
File src/gmm/gmm_prim.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/751b4541_e51ea1da
PS1, Line 107: gmm_llc_down_cb_dummy
`__func__`
I don't really expect this to
change but ok.
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/6b1e5bf5_b97f17bf
PS1, Line 467: rc = gprs_gmm_prim_handle_unsupported(gmm_prim);
This case can fall-through to default, but not
critical.
I did this on purpose since it's really the only primitive we expect
to have here, so the final logic is already in place.
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021/comment/a0943413_c28c7cac
PS1, Line 589: llc_prim
Why would anybody pass you NULL pointer here? Just
curious.
Why would anybody pass you NULL pointer here? Just curious.
You can apply this question to literally any ASSERT checking a null pointer in the whole
history of code.
--
To view, visit
https://gerrit.osmocom.org/c/libosmo-gprs/+/32021
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: I212053b3a3f27ef7d63503c3d5ef08453b2d2056
Gerrit-Change-Number: 32021
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 23 Mar 2023 10:29:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment