apply code review: refactor pfcp_endpoint API

Code review requested that the API should use functions instead of
direct access to a struct.

I have moved all user provided config to a separate struct
osmo_pfcp_endpoint_cfg, to be passed to osmo_pfcp_endpoint_create().
Halfway through those changes, I am not so certain whether that is what
reviewers had in mind. It makes sense from the point of view to keep nr
of arguments passed to osmo_pfcp_endpoint_create() small, and to allow
changing the user provided config without requiring a new
osmo_pfcp_endpoint_create2() API function. Though that again has ABI
compat problems, and makes no sense from the point of view that all
access should be done via API functions.

Personally I don't really agree with this change, which is probably the
reason why this patch ended up this way.

Related: SYS#5599
Change-Id: If80c35c6a942bf9593781b5a6bc28ba37323ce5e

2 files changed, 74 insertions, 13 deletions

diff --git a/src/libosmo-pfcp/pfcp_cp_peer.c b/src/libosmo-pfcp/pfcp_cp_peer.c
index e1dd9ac..959206a 100644
--- a/src/libosmo-pfcp/pfcp_cp_peer.c
+++ b/src/libosmo-pfcp/pfcp_cp_peer.c
@@ -161,7 +161,7 @@ static void pfcp_cp_peer_wait_assoc_setup_resp_onenter(struct osmo_fsm_inst *fi,
 	struct osmo_pfcp_msg *m;
 
 	m = osmo_pfcp_cp_peer_new_req(cp_peer, OSMO_PFCP_MSGT_ASSOC_SETUP_REQ);
-	m->ies.assoc_setup_req.recovery_time_stamp = cp_peer->ep->recovery_time_stamp;
+	m->ies.assoc_setup_req.recovery_time_stamp = osmo_pfcp_endpoint_get_recovery_timestamp(cp_peer->ep);
 
 	m->ies.assoc_setup_req.cp_function_features_present = true;
 	osmo_pfcp_bits_set(m->ies.assoc_setup_req.cp_function_features.bits, OSMO_PFCP_CP_FEAT_BUNDL, true);
diff --git a/src/libosmo-pfcp/pfcp_endpoint.c b/src/libosmo-pfcp/pfcp_endpoint.c
index 6162086..0ee3a9a 100644
--- a/src/libosmo-pfcp/pfcp_endpoint.c
+++ b/src/libosmo-pfcp/pfcp_endpoint.c
@@ -32,6 +32,29 @@
 #include <osmocom/pfcp/pfcp_endpoint.h>
 #include <osmocom/pfcp/pfcp_msg.h>
 
+/* Send/receive PFCP messages to/from remote PFCP endpoints. */
+struct osmo_pfcp_endpoint {
+	struct osmo_pfcp_endpoint_cfg cfg;
+
+	/* PFCP socket */
+	struct osmo_fd pfcp_fd;
+
+	/* The time at which this endpoint last restarted, as seconds since unix epoch. */
+	uint32_t recovery_time_stamp;
+
+	/* State for determining the next sequence number for transmitting a request message */
+	uint32_t seq_nr_state;
+
+	/* All transmitted PFCP Request messages, list of osmo_pfcp_queue_entry.
+	 * For a transmitted Request message, wait for a matching Response from a remote peer; if none arrives,
+	 * retransmit (see n1 and t1_ms). */
+	struct llist_head sent_requests;
+	/* All transmitted PFCP Response messages, list of osmo_pfcp_queue_entry.
+	 * For a transmitted Response message, keep it in the queue for a fixed amount of time. If the peer retransmits
+	 * the original Request, do not dispatch the Request, but respond with the queued message directly. */
+	struct llist_head sent_responses;
+};
+
 /*! Entry of pfcp_endpoint message queue of PFCP messages, for re-transsions. */
 struct osmo_pfcp_queue_entry {
 	/* entry in per-peer list of messages waiting for a response */
@@ -103,18 +126,22 @@ struct osmo_tdef osmo_pfcp_tdefs[] = {
 	{}
 };
 
-struct osmo_pfcp_endpoint *osmo_pfcp_endpoint_create(void *ctx, void *priv)
+/* Allocate a PFCP endpoint. Copy cfg's content to the allocated endpoint struct. Set the recovery_time_stamp to the
+ * current time. */
+struct osmo_pfcp_endpoint *osmo_pfcp_endpoint_create(void *ctx, const struct osmo_pfcp_endpoint_cfg *cfg)
 {
 	struct osmo_pfcp_endpoint *ep = talloc_zero(ctx, struct osmo_pfcp_endpoint);
 	uint32_t unix_time;
 	if (!ep)
 		return NULL;
 
+	ep->cfg = *cfg;
+	if (!ep->cfg.tdefs)
+		ep->cfg.tdefs = osmo_pfcp_tdefs;
+
 	INIT_LLIST_HEAD(&ep->sent_requests);
 	INIT_LLIST_HEAD(&ep->sent_responses);
 
-	ep->cfg.tdefs = osmo_pfcp_tdefs;
-	ep->priv = priv;
 	ep->pfcp_fd.fd = -1;
 
 	/* time() returns seconds since 1970 (UNIX epoch), but the recovery_time_stamp is coded in the NTP format, which is
@@ -343,8 +370,8 @@ static void osmo_pfcp_endpoint_handle_rx(struct osmo_pfcp_endpoint *ep, struct o
 
 		/* Populate message context to point at peer and session, if applicable.
 		 * With that context applied, log message rx. */
-		if (ep->set_msg_ctx)
-			ep->set_msg_ctx(ep, m, NULL);
+		if (ep->cfg.set_msg_ctx_cb)
+			ep->cfg.set_msg_ctx_cb(ep, m, NULL);
 		OSMO_LOG_PFCP_MSG(m, LOGL_INFO, "received retransmission of earlier request\n");
 
 		/* Also log on the earlier PFCP msg that it is resent */
@@ -362,8 +389,8 @@ static void osmo_pfcp_endpoint_handle_rx(struct osmo_pfcp_endpoint *ep, struct o
 
 	/* Populate message context to point at peer and session, if applicable.
 	 * With that context applied, log message rx. */
-	if (ep->set_msg_ctx)
-		ep->set_msg_ctx(ep, m, req);
+	if (ep->cfg.set_msg_ctx_cb)
+		ep->cfg.set_msg_ctx_cb(ep, m, req);
 	OSMO_LOG_PFCP_MSG(m, LOGL_INFO, "received\n");
 
 	if (req && req->ctx.resp_cb) {
@@ -373,12 +400,12 @@ static void osmo_pfcp_endpoint_handle_rx(struct osmo_pfcp_endpoint *ep, struct o
 		if (rc != 1) {
 			dispatch_rx = false;
 			OSMO_LOG_PFCP_MSG(m, LOGL_DEBUG,
-					  "response handled by m->resp_cb(), not dispatching to rx_msg()\n");
+					  "response handled by m->resp_cb(), not dispatching to rx_msg_cb()\n");
 		}
 	}
 
 	if (dispatch_rx)
-		ep->rx_msg(ep, m, req);
+		ep->cfg.rx_msg_cb(ep, m, req);
 	if (req)
 		osmo_pfcp_queue_del(prev_msg);
 }
@@ -402,7 +429,7 @@ static int osmo_pfcp_fd_cb(struct osmo_fd *ofd, unsigned int what)
 			return -EIO;
 		msgb_put(msg, rc);
 
-		OSMO_ASSERT(ep->rx_msg);
+		OSMO_ASSERT(ep->cfg.rx_msg_cb);
 
 		/* This may be a bundle of PFCP messages. Parse and receive each message received, by shifting l4h
 		 * through the message bundle. */
@@ -436,8 +463,8 @@ int osmo_pfcp_endpoint_bind(struct osmo_pfcp_endpoint *ep)
 	/* close the existing socket, if any */
 	osmo_pfcp_endpoint_close(ep);
 
-	if (!ep->rx_msg) {
-		LOGP(DLPFCP, LOGL_ERROR, "missing rx_msg cb at osmo_pfcp_endpoint\n");
+	if (!ep->cfg.rx_msg_cb) {
+		LOGP(DLPFCP, LOGL_ERROR, "missing cfg.rx_msg_cb at osmo_pfcp_endpoint\n");
 		return -EINVAL;
 	}
 
@@ -483,3 +510,37 @@ void osmo_pfcp_endpoint_invalidate_ctx(struct osmo_pfcp_endpoint *ep, struct osm
 	llist_for_each_entry(qe, &ep->sent_responses, entry)
 		osmo_pfcp_msg_invalidate_ctx(qe->m, deleted_fi);
 }
+
+/* Return the cfg for an endpoint, guaranteed to return non-NULL for a valid ep. */
+const struct osmo_pfcp_endpoint_cfg *osmo_pfcp_endpoint_get_cfg(const struct osmo_pfcp_endpoint *ep)
+{
+	return &ep->cfg;
+}
+
+/* Shorthand for &osmo_pfcp_endpoint_get_cfg(ep)->priv */
+void *osmo_pfcp_endpoint_get_priv(const struct osmo_pfcp_endpoint *ep)
+{
+	return ep->cfg.priv;
+}
+
+uint32_t osmo_pfcp_endpoint_get_recovery_timestamp(const struct osmo_pfcp_endpoint *ep)
+{
+	return ep->recovery_time_stamp;
+}
+
+/* Shorthand for &osmo_pfcp_endpoint_get_cfg(ep)->local_addr */
+const struct osmo_sockaddr *osmo_pfcp_endpoint_get_local_addr(const struct osmo_pfcp_endpoint *ep)
+{
+	return &ep->cfg.local_addr;
+}
+
+void osmo_pfcp_endpoint_set_seq_nr_state(struct osmo_pfcp_endpoint *ep, uint32_t seq_nr_state)
+{
+	ep->seq_nr_state = seq_nr_state;
+}
+
+/* Return true when the retransmission queues contain any PFCP messages, false when the queues are empty. */
+bool osmo_pfcp_endpoint_retrans_queue_is_busy(const struct osmo_pfcp_endpoint *ep)
+{
+	return !(llist_empty(&ep->sent_requests) && llist_empty(&ep->sent_responses));
+}