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?