Attention is currently required from: daniel, lynxis lazus. laforge 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 11: (6 comments) File gtp/gtp.c: https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/e2e69e10_106b6071?usp=email : PS11, Line 899: struct Im surprised that the destination peer is not 'const'. Likeweise I'd normally expect any othe rinput data like local_ref or ie to be 'const' as the called function shouldn't modify any of these. Probably you're doing it this way as a lot / all of the existing code does it this way? https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/9712d78b_434ec111?usp=email : PS11, Line 1134: int gtp_decode_pdp_ctx(const uint8_t *buf, unsigned int size, struct pdp_t *pdp, uint16_t *sapi) I think the function could deserve some documentation as a comment [doxygen style or not] https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/b510fb7e_0eb96e0f?usp=email : PS11, Line 1273: int gtp_encode_pdp_ctx(uint8_t *buf, unsigned int size, const struct pdp_t *pdp, uint16_t sapi) I think the function could deserve some documentation as a comment [doxygen style or not] https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/3a753755_77d7eb15?usp=email : PS11, Line 1403: LOGP(DLGTP, LOGL_NOTICE, shouldn't we print some kind of context? Like which GSN peer (address) has sent us the message with the wrong version? https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/2acd816c_11141355?usp=email : PS11, Line 1410: "SGSN Context Response expected Sequence flag set. Flags %d\n", packet->flags); shouldn't we print some kind of context? Like which GSN peer (address) has sent us the message with the wrong flags? https://gerrit.osmocom.org/c/osmo-ggsn/+/38938/comment/4777d45d_0b8ecfc0?usp=email : PS11, Line 1458: "SGSN Context Request expected only on GTPCv1: %u\n", version); same here -- 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: 11 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: Tue, 25 Feb 2025 12:11:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
