5 Jun
2025
5 Jun
'25
8:21 a.m.
Attention is currently required from: daniel, lynxis lazus.
pespin has posted comments on this change by lynxis lazus. (
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938?usp=email )
Change subject: gtp: add support for SGSN Context Req/Resp/Ack
......................................................................
Patch Set 18:
(15 comments)
File gtp/gtp.c:
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/3c656190_1c4b1b4e?us… :
PS18, Line 400: union gtp_packet *packet, int len,
indentation
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/94d68fac_34b652cd?us… :
PS18, Line 888: const struct sockaddr_in *peer, union gtpie_member **ie, size_t
ie_size)
the "**ie" contents can be consitfied like you did in a previous patch right?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/3300be40_0e35c5ef?us… :
PS18, Line 894: union gtpie_member resp_ie_elem[2] = {};
req_ie_elem?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/d60eae7c_7036ac66?us… :
PS18, Line 925: union gtpie_member **ie, unsigned int ie_size)
constify?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/fbaefd92_d60ef6ab?us… :
PS18, Line 967: union gtpie_member *ie[GTP_MAX] = {};
why GTP_MAX elements if you are only configuring one below?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/2a6a4083_fe8e046b?us… :
PS18, Line 981: union gtpie_member **ie, unsigned int ie_size)
constify
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/0b94a0e4_a2f693e4?us… :
PS18, Line 1203: /* FIXME: parse PDP Address */
what about this? afaiu it's done bleow already?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/8130bc5b_894a5337?us… :
PS18, Line 1217: /* FIXME: check for correct type and length */
what about this?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/252ce551_c288d51d?us… :
PS18, Line 1429: /* FIXME: retransmission need to be implemented:
afaiu this is already done by gtp_conf()?
File gtp/gtp_sgsn_ctx.h:
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/5c36c7a1_6a5b549c?us… :
PS18, Line 25: /* remote SGSN/MME request a Ctx from this peer */
adding a whitespace line before this one may help understand there are 2 blocks of
states.
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9497cc3c_be53a3c7?us… :
PS18, Line 36: SGSN_CTX_REQ_E_RX_REQ,
I'd welcome if you can document here the type of value being passed as a parameter for
each event expecting one. I find this incredibly useful when reading FSMs and making sure
everything is fine.
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/5cbf9afc_2ba1519f?us… :
PS18, Line 52: struct llist_head local_reqs;
we'll probably need some sort of hashtable here later on.
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/892085d1_7fa21a08?us… :
PS18, Line 67: struct osmo_fsm_inst *fsm;
can we call this "fi" like virtually everywhere in our code base?
File gtp/gtp_sgsn_ctx.c:
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/950faa3f_268b016c?us… :
PS18, Line 325: llist_del(&req->list);
req is not added to any llist during _alloc(), which means if user calls "req =
sgsn_ctx_alloc(); sgsn_ctx_free(req);" weird stuff may happen?
https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9aed7755_f6110f53?us… :
PS18, Line 439: /* TODO: move tx GTPv1 into the fsm */
what about this and similar ones below?
--
To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/38938?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-ggsn
Gerrit-Branch: master
Gerrit-Change-Id: Idb8ed0bb87200a68bb8caedd734fc070df9179c0
Gerrit-Change-Number: 38938
Gerrit-PatchSet: 18
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Thu, 05 Jun 2025 12:21:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No