Attention is currently required from: neels. pespin 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: (7 comments) File include/osmocom/pfcp/pfcp_msg.h: https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/9bd8f6d3_249ab9dd PS3, Line 44: #define OSMO_LOG_PFCP_MSG_SRC(M, LEVEL, file, line, FMT, ARGS...) do { \ This starts to look as too much for a macro imho, it may make sense to move it to a variadic function https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/c6227464_17a86ef5 PS3, Line 69: static inline uint32_t osmo_pfcp_next_seq(uint32_t *seq_state) This function will return 1 on first call most probably. Are you sure that's accepted per spec? or it needs to starts with 0? https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/2d00fcc1_188ddd66 PS3, Line 110: struct osmo_fsm_inst *peer_fi; Looks like these 2 groups can be in a union? https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/a4266ec8_93fcbdcb PS3, Line 123: #define OSMO_PFCP_MSG_FOR_IES(IES_P) ((struct osmo_pfcp_msg *)((char *)IES_P - offsetof(struct osmo_pfcp_msg, ies))) what's the meaning of FOR here? https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/6d8ae394_271aff96 PS3, Line 149: #define OSMO_PFCP_MSG_MEMB(M, OFS) ((OFS) <= 0 ? NULL : (void *)((uint8_t *)(M) + OFS)) write it as a static inline, where you can set type of OFS to unsigned, you can then drop the first check. Or you reall expect to pass -1 to it? File src/libosmo-pfcp/pfcp_msg.c: https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/7e3ccae4_d05fa19f PS3, Line 72: uint16_t message_length; You may want to use here: union { struct osmo_pfcp_header_no_seid hdr_no_seid[0]; struct osmo_pfcp_header_seid hdr_seid[0]; } https://gerrit.osmocom.org/c/osmo-upf/+/27631/comment/063ee253_e4e1f759 PS3, Line 338: /* Append the encoded PFCP message to the message buffer. I see a lot of stuff to put several messages into one buffer. Is there a specific reason to need that? -- 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-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Comment-Date: Mon, 04 Apr 2022 09:08:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment