Attention is currently required from: laforge, fixeria, pespin.
9 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 { \
Agree, let's avoid over bloated macros please.
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.
Patch Set #3, Line 69: static inline uint32_t osmo_pfcp_next_seq(uint32_t *seq_state)
I would expect no constraint whatsoevre on the starting / initial value of the sequence number. […]
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?
Patch Set #3, Line 72: (*seq_state) &= 0xffffff;
the interesting question is why we use a uint32_t variable if we only use 16 bits of it?
that's 24 bits.
osmo_pfcp_header_parsed stores a uint32_t, on the wire it is 24 bits wide.
Patch Set #3, Line 110: struct osmo_fsm_inst *peer_fi;
peers and sessins are orthogonal concepts, why do you think a message is only either part of a sessi […]
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.
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?
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.
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 […]
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:
Patch Set #3, Line 65: struct osmo_pfcp_header_common {
Don't you need to add conditional blocks for little/big endian here?
oh yes, forgot to run that script...
Patch Set #3, Line 72: uint16_t message_length;
You may want to use here: […]
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...
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. […]
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 change 27631. To unsubscribe, or for help writing mail filters, visit settings.