Change in osmo-upf[master]: add pfcp_endpoint - gerrit-log

14 Jun 2022

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 &lt;nhofmeyr(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: neels &lt;nhofmeyr(a)sysmocom.de&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Comment-Date: Tue, 14 Jun 2022 19:27:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels &lt;nhofmeyr(a)sysmocom.de&gt;
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-MessageType: comment