[XL] Change in osmo-ggsn[master]: gtp: add support for SGSN Context Req/Resp/Ack - gerrit-log

5 Jul 2025

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?us… :
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?us… :
PS18, Line 400: 		   union gtp_packet *packet, int len,
...
  indentation Done

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/01712082_610a06b3?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? no. it gets modified.

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/e32d80c4_8b1c3275?us… :
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?us… :
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?us… :
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?us… :
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?us… :
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?us… :
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?us… :
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?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. Done

https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9038ccf5_fb19520f?us… :
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?us… :
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?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(); 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?us… :
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 &lt;lynxis(a)fe80.eu&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-Comment-Date: Fri, 04 Jul 2025 23:21:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt;
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;