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 3:

(14 comments)

File include/osmocom/pfcp/pfcp_endpoint.h:

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/af4b7e80_6a8b3623 PS3, Line 46: * \param req If m is a PFCP Response to an earlier Request, req is that request message. */ otherwise NULL?

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/1affc898_45259753 PS3, Line 51: struct osmo_pfcp_endpoint { osmo_pfcp_endp seems like it's going to be a typing/reading change worth it ;)

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/d546a61b_ef4e0142 PS3, Line 77: osmo_pfcp_endpoint_cb set_msg_ctx; can we call all these callbacks with _cb at the end?

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/35f7180e_1416176a 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.

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/95ea74dc_d876c9b8 PS3, Line 104: In general I'm missing some APIs to set callbaks, local_node_id, etc.

File src/libosmo-pfcp/pfcp_endpoint.c:

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/cc33a298_f66517f2 PS3, Line 42: struct osmo_pfcp_msg *m; Doesn't m already have a backpointer to ep? May make more sense to just have that one.

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/3937e30a_9642a8b2 PS3, Line 44: struct osmo_timer_list t1; This can be probably optimized to be only 1 timer per list, by moving them to the end of the list when transmitting.

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/666950be_1d455595 PS3, Line 73: int osmo_pfcp_queue_destructor(struct osmo_pfcp_queue_entry *qe) why is the destructor not static?

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/fa7cf025_76293f47 PS3, Line 81: { .T = -19, .default_val = 15, .unit = OSMO_TDEF_S, why some timers have names and some don't?

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/53eeee82_dfbaa664 PS3, Line 122: /* time() returns seconds since 1970 (UNIX epoch), but the recovery_time_stamp is coded in the NTP format, which is you can check open5gs code, iirc some stuff exists to handle ntp timestamps and I also added some bits for Gy interface.

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/88bfb347_466a7bef PS3, Line 139: unsigned int ep_t1(struct osmo_pfcp_endpoint *ep) static

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/5003a26e_76020902 PS3, Line 144: unsigned int ep_keep_resp(struct osmo_pfcp_endpoint *ep) static

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/6b21310a_656371d0 PS3, Line 177: static void pfcp_queue_timer_cb(void *data) Maybe rename to contain "expiry" in it?

https://gerrit.osmocom.org/c/osmo-upf/+/28244/comment/19fc2ff0_ed14639d PS3, Line 182: if (qe->m->is_response) { I see you are basically putting together in the same list 2 different things: * List of transmitted request messages (in order to retransmit them at interval times). * List of transmitted response messages (no retrans happening, only keeping them in order to match and resend when a retransmit req arrives).

I don't see the point in having them in the same list, because they are looked up and used at different scenarios. Having it in different lists means you don't need to loop over all requests+reposnses every time, only about the important subset (req or resp). See for instance how libgtp does it, since I believe it's the same system as in GTP.