Attention is currently required from: neels, pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-upf/+/28244 )
Change subject: add pfcp_endpoint ...................................................................... Patch Set 4: (7 comments) File include/osmocom/pfcp/pfcp_endpoint.h: https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/50144b27_ee15e654 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 t […] not sure why endp is supposed to be better than endpoint? Does it matter? https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/7b29a519_7ebdaa09 PS3, Line 77: osmo_pfcp_endpoint_cb set_msg_ctx; > can we call all these callbacks with _cb at the end? agreed https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/47d23ff1_8137b681 PS3, Line 99: int osmo_pfcp_endpoint_tx(struct osmo_pfcp_endpoint *ep, struct osmo_pfcp_msg *m); > something more meaningful like "pfcp_msg" instead of "m" may be worth a > change. does it matter in a prototype? https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/b80ff51f_d736cfff PS3, Line 104: > You call it API bloat, I call it do not break ABI next time you add something > new in eg. […] In general, the question is whether 'struct osmo_pfcp_endpoint' is supposed to be publicly exposed to every application. At this point you will have a very tight ABI coupling. The clean and safe approach is to go for what pespin is asking. In Osmocom we don't have a clear policy on what should be done. If we expect the struct will still be in flux a bit in the forseeable future, it might be an extra reason to go for the setter/getter functions. File src/libosmo-pfcp/pfcp_endpoint.c: https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/8e4d1cab_55463bd5 PS3, Line 44: struct osmo_timer_list t1; > that's the point of the osmo_timer API: it keeps a list and waits for the > first entry, later timers […] Done https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/95eec113_07e449b8 PS3, Line 177: static void pfcp_queue_timer_cb(void *data) > "timer_cb" is consistent: for example in osmo_fsm, the name is timer_cb, and > it means that a timer h […] Done File src/libosmo-pfcp/pfcp_endpoint.c: https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/d83091a9_ea992973 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. […] I haven't done a deep review, but if there is no reason to have a shared path/queue for requests and responses, I would also think it's more logical to keep them separate. -- 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Tue, 14 Jun 2022 19:27:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
