Attention is currently required from: laforge, fixeria, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-upf/+/27631 ) Change subject: libosmo-pfcp: implement PFCP header and msg handling ...................................................................... Patch Set 3: (9 comments) File include/osmocom/pfcp/pfcp_msg.h: https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/cbc1654c_1e49a1e5 PS3, Line 44: #define OSMO_LOG_PFCP_MSG_SRC(M, LEVEL, file, line, FMT, ARGS...) do { \ you are aware of the "ARGS..." argument, are you sure you want this to be a function with a va_list? It would be the first time we'd not use a macro for LOGP() style functionality, afaik. https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/7f927825_898265ce PS3, Line 69: static inline uint32_t osmo_pfcp_next_seq(uint32_t *seq_state) The seq nr can be chosen freely. I want it to start with 1 because 0 happens when a struct is zero initialized. I guess I also want it to not wrap to 0 then? https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/13f4be4f_0b1d9503 PS3, Line 72: (*seq_state) &= 0xffffff; that's 24 bits. osmo_pfcp_header_parsed stores a uint32_t, on the wire it is 24 bits wide. https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/9e7ce90a_ff801eb0 PS3, Line 110: struct osmo_fsm_inst *peer_fi; if it is only related to a peer (Association Setup / Update, etc), session == NULL. If it is related to a session, it is also related to a peer that the session is set up with. Both pointers are used to reduce lookup iterations. endpoint +- peer +- session (If related to a session, this more precisely shows where the message belongs, so the logging macro prefers the session fi to log on. Still the peer fi will point to the correct peer as well.) BTW, these are optionally used by the calling code, they may also remain NULL entirely. https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/5e1a2400_2038e37d PS3, Line 123: #define OSMO_PFCP_MSG_FOR_IES(IES_P) ((struct osmo_pfcp_msg *)((char *)IES_P - offsetof(struct osmo_pfcp_msg, ies))) there is the osmo_pfcp_msg->ies member. Given the pointer to ies, get the containing osmo_pfcp_msg. So get the msg "for" an ies union. the FOR literally means 'for', could also be FROM? Adding a comment. https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/d8cb27b5_1c737206 PS3, Line 149: #define OSMO_PFCP_MSG_MEMB(M, OFS) ((OFS) <= 0 ? NULL : (void *)((uint8_t *)(M) + OFS)) for example, some messages don't have a Cause IE. So if osmo_pfcp_msg->ofs_cause is 0 or < 0, I want to get NULL returned. File src/libosmo-pfcp/pfcp_msg.c: https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/0bb42391_a3f66150 PS3, Line 65: struct osmo_pfcp_header_common { oh yes, forgot to run that script... https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/4b6ace2a_8c137c7b PS3, Line 72: uint16_t message_length; i think i made separate structs to get a nice sizeof() of the common part. hmm, zero length arrays would work for that. but then i can't store anything in those. I'll see if it makes the code simpler... https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/a88d7cf3_67f2d372 PS3, Line 338: /* Append the encoded PFCP message to the message buffer. It is a feature described in the PFCP specs. Only after implementing did I notice that we could simply indicate feature BUNDL==0 and thus tell the other side that we need each message in its own packet. So now by accident we have the ability to parse bundled PFCP and let's say we are ready to implement encoding of bundled PFCP. Might turn out to be a good thing when interworking with high throughput components? -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/27631 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: I3f85ea052a6b7c064244a8093777e53a47c8c61e Gerrit-Change-Number: 27631 Gerrit-PatchSet: 3 Gerrit-Owner: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-CC: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Comment-Date: Wed, 06 Apr 2022 23:13:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt; Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: comment