Attention is currently required from: neels.
5 comments:
File include/osmocom/pfcp/pfcp_endpoint.h:
Patch Set #3, Line 51: struct osmo_pfcp_endpoint {
you mean the name should change to "osmo_pfcp_endp"? […]
Given this probably ends up as a public API in a shared library I think this is precisely the time to pinpoint this kind of stuff.
Do you mean add getters and setters? […]
You call it API bloat, I call it do not break ABI next time you add something new in eg. osmo_pfcp_endpoint.cfg
Those callbacks are only set once and then used internally, so adding a setter API to add those makes sense. This way you avoid ABI breakage.
File src/libosmo-pfcp/pfcp_endpoint.c:
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
i'm pretty unsure about this timestamp coding, just know that wireshark ended up showing the expecte […]
Grep for "ntp32" in there.
Patch Set #3, Line 182: if (qe->m->is_response) {
Let me rephrase your comment: […]
IMHO it also makes sense to have 2 different timer callbacks, one for requests and another for responses. You are unnecessarily still mixing stuff in the same code path here.
File src/libosmo-pfcp/pfcp_endpoint.c:
Patch Set #4, Line 268: /* Slight optimization: Add sent requests to the start of the list: we will usually receive a response shortly
You say to the start of the queue, but you do add_tail in both.
Does PFCP actually retransmit responses actively? I don't think so?
Why do you have a common osmo_pfcp_endpoint_retrans_queue_add()? Again, it makes sense to have completely separate paths for responses and requests here, they are queued for totally different reasons.
To view, visit change 28244. To unsubscribe, or for help writing mail filters, visit settings.