Attention is currently required from: pespin.
12 comments:
File include/osmocom/pfcp/pfcp_endpoint.h:
Patch Set #3, Line 46: * \param req If m is a PFCP Response to an earlier Request, req is that request message. */
otherwise NULL?
ACK
Patch Set #3, Line 51: struct osmo_pfcp_endpoint {
osmo_pfcp_endp seems like it's going to be a typing/reading change worth it ;)
you mean the name should change to "osmo_pfcp_endp"?
Can we focus on functional code review please?
In general I'm missing some APIs to set callbaks, local_node_id, etc.
Do you mean add getters and setters?
I'm not going to add API bloat for each struct member.
File src/libosmo-pfcp/pfcp_endpoint.c:
Patch Set #3, Line 42: struct osmo_pfcp_msg *m;
Doesn't m already have a backpointer to ep? May make more sense to just have that one.
no, osmo_pfcp_msg has no pointer to an endpoint. Also it shouldn't.
Patch Set #3, Line 44: struct osmo_timer_list t1;
This can be probably optimized to be only 1 timer per list, by moving them to the end of the list wh […]
that's the point of the osmo_timer API: it keeps a list and waits for the first entry, later timers untouched behind it in the list.
(Recently I was surprised to see load caused by timers triggering every 10th of a second; then again that was multiplied by number of active lchans. PFCP messages are more like 5 seconds timeouts, and not firing continuously. I see no need to optimize here.)
Patch Set #3, Line 73: int osmo_pfcp_queue_destructor(struct osmo_pfcp_queue_entry *qe)
why is the destructor not static?
don't know why.
it should be static, thanks.
Patch Set #3, Line 81: { .T = -19, .default_val = 15, .unit = OSMO_TDEF_S,
why some timers have names and some don't?
there is no reason why.
We use magic numbers for T = N almost everywhere in osmocom.
But setting names made me notice a mistake, 27 should be 21, thx
Patch Set #3, Line 122: /* time() returns seconds since 1970 (UNIX epoch), but the recovery_time_stamp is coded in the NTP format, which is
you can check open5gs code, iirc some stuff exists to handle ntp timestamps and I also added some bi […]
i'm pretty unsure about this timestamp coding, just know that wireshark ended up showing the expected date.
do you have more specific code pointers?
Or even, is there API I can call instead of re-implementing?
OTOH the accuracy of the timestamp isn't really important AFAICT, the point of this is to have a unique value per application restart, for peers to detect that the peer has lost its state since last time. It's not critical to get this perfectly accurate, at least not until someone complains about it.
Patch Set #3, Line 139: unsigned int ep_t1(struct osmo_pfcp_endpoint *ep)
static
thx
Patch Set #3, Line 144: unsigned int ep_keep_resp(struct osmo_pfcp_endpoint *ep)
static
thx
(I'm dreaming of a tool that tells us which unused #includes we have in .c and .h files, and which functions should be made static because used only in a single .c file... something like this should exist right? maybe the linter has such features??)
Patch Set #3, Line 177: static void pfcp_queue_timer_cb(void *data)
Maybe rename to contain "expiry" in it?
"timer_cb" is consistent: for example in osmo_fsm, the name is timer_cb, and it means that a timer has expired
Patch Set #3, Line 182: if (qe->m->is_response) {
I see you are basically putting together in the same list 2 different things: […]
Let me rephrase your comment:
It would be more optimal to have separate lists for requests and responses.
You're right, thanks
To view, visit change 28244. To unsubscribe, or for help writing mail filters, visit settings.