Attention is currently required from: neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-upf/+/28244 )
Change subject: add pfcp_endpoint ......................................................................
Patch Set 4:
(5 comments)
File include/osmocom/pfcp/pfcp_endpoint.h:
https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/ef9f36f5_883e1392 PS3, 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.
https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/6274b4f3_4665da4a PS3, Line 104:
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:
https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/32653b2b_417a78dc PS3, 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.
https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/0bdd32a2_08ff7d97 PS3, 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:
https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/4d7b0db4_2bed5e55 PS4, 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.
-
pespin