Attention is currently required from: neels.

View Change

7 comments:

• File include/osmocom/pfcp/pfcp_msg.h:

  • Patch Set #3, 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

  • Patch Set #3, 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?

  • Patch Set #3, Line 110: struct osmo_fsm_inst *peer_fi;

    Looks like these 2 groups can be in a union?

  • Patch Set #3, 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?

  • Patch Set #3, 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:

  • Patch Set #3, 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];
    }

  • Patch Set #3, 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 change 27631. To unsubscribe, or for help writing mail filters, visit settings.