Attention is currently required from: daniel, laforge, pespin. lynxis lazus 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 20: (15 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/ced5c66e_f8be7790?usp=email : PS18, Line 11: the API for the SGSN which uses libgtp as well as the external SGSN which communicates > cosmetic: too long commit log; your local git client should likely have > warned you about it already? No, my git client doesn't warn me about it. I'll check how to enable it. File gtp/gtp.c: https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/86ba9f2e_a86c4f94?usp=email : PS18, Line 400: union gtp_packet *packet, int len, > indentation Done https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/01712082_610a06b3?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? no. it gets modified. https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/e32d80c4_8b1c3275?usp=email : PS18, Line 894: union gtpie_member resp_ie_elem[2] = {}; > req_ie_elem? Done https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/57ce1f68_8368b643?usp=email : PS18, Line 925: union gtpie_member **ie, unsigned int ie_size) > constify? no. it gets modified. https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/80cd35a2_c26410c6?usp=email : PS18, Line 967: union gtpie_member *ie[GTP_MAX] = {}; > why GTP_MAX elements if you are only configuring one below? Because of the GTP code base. I've removed ie_size/ie_len field from the outside api because the whole internal code expect to have an ie[GTPIE_SIZE]. I dislike this behavior, but don't see a good point in fixing this here. https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/bab2186f_3d9709c1?usp=email : PS18, Line 981: union gtpie_member **ie, unsigned int ie_size) > constify I would like to allow the fsm to modify the list of ies on the way to the wire. Or I have to copy the whole list. https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/16abb64f_215b51e4?usp=email : PS18, Line 1203: /* FIXME: parse PDP Address */ > what about this? afaiu it's done bleow already? Done https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/fd68c81f_b2e6b423?usp=email : PS18, Line 1217: /* FIXME: check for correct type and length */ > what about this? Done https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/433b7c9a_763b6ad2?usp=email : PS18, Line 1429: /* FIXME: retransmission need to be implemented: > afaiu this is already done by gtp_conf()? I need to write a test case for and check if gtp_conf() is really re-transmitting. The SGSN Req/Resp/Ack is a little bit special in this regard, because they are 3 packets instead of 2 (Req/Resp). File gtp/gtp_sgsn_ctx.h: https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/7b6ff65f_9fcd98cd?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. Done https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9038ccf5_fb19520f?usp=email : PS18, Line 52: struct llist_head local_reqs; > we'll probably need some sort of hashtable here later on. Sure we could switch to a hash map. But there shouldn't be a huge list of outstanding SGSN contexts around. https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/0a08b041_487c4d32?usp=email : PS18, Line 67: struct osmo_fsm_inst *fsm; > can we call this "fi" like virtually everywhere in our code base? Done File gtp/gtp_sgsn_ctx.c: https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/20fe9a60_464b202e?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(); sg […] It is added to a list by sgsn_ctx_req_alloc_outgoing or *sgsn_ctx_req_alloc_incoming. sgsn_ctx_req_alloc() is never directly called except to one of those. I'll add an underscope to the function to make it more clear. https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/34a75f76_f1502df3?usp=email : PS18, Line 439: /* TODO: move tx GTPv1 into the fsm */ > what about this and similar ones below? I'm currently not very happy with this approach. The fsm doesn't transmit the packet, it only validates a packet, but not transmitting it. -- 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: 20 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: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Comment-Date: Fri, 04 Jul 2025 23:21:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <[email protected]> Comment-In-Reply-To: pespin <[email protected]>
