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?usp=email : PS18, Line 400: union gtp_packet *packet, int len, indentation https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/94d68fac_34b652cd?usp=email : 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?usp=email : PS18, Line 894: union gtpie_member resp_ie_elem[2] = {}; req_ie_elem? https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/d60eae7c_7036ac66?usp=email : PS18, Line 925: union gtpie_member **ie, unsigned int ie_size) constify? https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/fbaefd92_d60ef6ab?usp=email : 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?usp=email : PS18, Line 981: union gtpie_member **ie, unsigned int ie_size) constify https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/0b94a0e4_a2f693e4?usp=email : 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?usp=email : PS18, Line 1217: /* FIXME: check for correct type and length */ what about this? https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/252ce551_c288d51d?usp=email : 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?usp=email : 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?usp=email : 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?usp=email : 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?usp=email : 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?usp=email : 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?usp=email : 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: daniel <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Attention: lynxis lazus <[email protected]> Gerrit-Comment-Date: Thu, 05 Jun 2025 12:21:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
