Change in osmo-upf[master]: add pfcp_endpoint

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 { 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: 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 Grep for "ntp32" in there. https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/0bdd32a2_08ff7d97 PS3, Line 182: if (qe->m->is_response) { 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. -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/28244 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: Ic8d42e201b63064a71b40ca45a5a40e29941e8ac Gerrit-Change-Number: 28244 Gerrit-PatchSet: 4 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Fri, 10 Jun 2022 09:34:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment