neels has uploaded this change for review. (
https://gerrit.osmocom.org/c/libosmo-pfcp/+/28754 )
Change subject: apply code review: refactor pfcp_endpoint API
......................................................................
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
---
M include/osmocom/pfcp/pfcp_endpoint.h
M src/libosmo-pfcp/pfcp_cp_peer.c
M src/libosmo-pfcp/pfcp_endpoint.c
3 files changed, 106 insertions(+), 56 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-pfcp refs/changes/54/28754/1
diff --git a/include/osmocom/pfcp/pfcp_endpoint.h
b/include/osmocom/pfcp/pfcp_endpoint.h
index acc878e..817cd0f 100644
--- a/include/osmocom/pfcp/pfcp_endpoint.h
+++ b/include/osmocom/pfcp/pfcp_endpoint.h
@@ -32,13 +32,15 @@
struct osmo_pfcp_endpoint;
struct osmo_fsm_inst;
-#define OSMO_PFCP_TIMER_HEARTBEAT_REQ -19
-#define OSMO_PFCP_TIMER_HEARTBEAT_RESP -20
-#define OSMO_PFCP_TIMER_GRACEFUL_REL -21
-#define OSMO_PFCP_TIMER_T1 -22
-#define OSMO_PFCP_TIMER_N1 -23
-#define OSMO_PFCP_TIMER_KEEP_RESP -24
-#define OSMO_PFCP_TIMER_ASSOC_RETRY -26
+enum osmo_pfcp_timers {
+ OSMO_PFCP_TIMER_HEARTBEAT_REQ = -19,
+ OSMO_PFCP_TIMER_HEARTBEAT_RESP = -20,
+ OSMO_PFCP_TIMER_GRACEFUL_REL = -21,
+ OSMO_PFCP_TIMER_T1 = -22,
+ OSMO_PFCP_TIMER_N1 = -23,
+ OSMO_PFCP_TIMER_KEEP_RESP = -24,
+ OSMO_PFCP_TIMER_ASSOC_RETRY = -26,
+};
extern struct osmo_tdef osmo_pfcp_tdefs[];
@@ -51,51 +53,37 @@
struct osmo_pfcp_msg *req);
/* Send/receive PFCP messages to/from remote PFCP endpoints. */
-struct osmo_pfcp_endpoint {
- struct {
- /* Local address */
- struct osmo_sockaddr local_addr;
- /* Local PFCP Node ID, as sent in outgoing messages' Node ID IE
*/
- struct osmo_pfcp_ie_node_id local_node_id;
+struct osmo_pfcp_endpoint;
- /* Timer definitions to use, if any. See t1_ms, keep_resp_ms.
Use osmo_pfcp_tdefs by default. It is
- * convenient to add osmo_pfcp_tdefs as one of your program's
osmo_tdef_group entries and call
- * osmo_tdef_vty_init() to expose PFCP timers on the VTY. */
- const struct osmo_tdef *tdefs;
- } cfg;
+struct osmo_pfcp_endpoint_cfg {
+ /* Local address */
+ struct osmo_sockaddr local_addr;
+ /* Local PFCP Node ID, as sent in outgoing messages' Node ID IE */
+ struct osmo_pfcp_ie_node_id local_node_id;
- /* PFCP socket */
- struct osmo_fd pfcp_fd;
+ /* If non-NULL, this function is called just after decoding and before
handling the osmo_pfcp_msg passed as
+ * argument m.
+ * The caller (you) usually implements this to set m->ctx.peer_fi and
m->ctx.session_fi as appropriate,
+ * so that these are used for logging context during message handling.
The caller may also use m->ctx.peer_fi
+ * and m->ctx.session_fi pointers to reduce lookup iterations in e.g.
rx_msg(). */
+ osmo_pfcp_endpoint_cb set_msg_ctx_cb;
- /* The time at which this endpoint last restarted, as seconds since
unix epoch. */
- uint32_t recovery_time_stamp;
+ /* Callback to receive a single incoming PFCP message from a remote
peer, already decoded. See also the doc for
+ * osmo_pfcp_endpoint_cb. */
+ osmo_pfcp_endpoint_cb rx_msg_cb;
- /* State for determining the next sequence number for transmitting a
request message */
- uint32_t seq_nr_state;
-
- /* This function is called just after decoding and before handling the
message.
- * This function may set ctx.peer_fi and ctx.session_fi, used for
logging context during message decoding.
- * The caller may also use these fi pointers to reduce lookup
iterations in rx_msg().
- */
- osmo_pfcp_endpoint_cb set_msg_ctx;
-
- /* Callback to receive single incoming PFCP messages from a remote
peer, already decoded. */
- osmo_pfcp_endpoint_cb rx_msg;
+ /* Custom timer definitions to use, if any. Relevant timers are:
OSMO_PFCP_TIMER_N1, OSMO_PFCP_TIMER_T1,
+ * OSMO_PFCP_TIMER_KEEP_RESP. These are used for the PFCP message
retransmission queue.
+ * If passed NULL, use the timer definitions from the global
osmo_pfcp_tdefs.
+ * To expose retransmission timers on the VTY configuration, it is
convenient to add osmo_pfcp_tdefs as one of
+ * your program's osmo_tdef_group entries and call
osmo_tdef_vty_init(). */
+ const struct osmo_tdef *tdefs;
/* application-private data */
void *priv;
-
- /* 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;
};
-struct osmo_pfcp_endpoint *osmo_pfcp_endpoint_create(void *ctx, void *priv);
+struct osmo_pfcp_endpoint *osmo_pfcp_endpoint_create(void *ctx, const struct
osmo_pfcp_endpoint_cfg *cfg);
int osmo_pfcp_endpoint_bind(struct osmo_pfcp_endpoint *ep);
void osmo_pfcp_endpoint_close(struct osmo_pfcp_endpoint *ep);
void osmo_pfcp_endpoint_free(struct osmo_pfcp_endpoint **ep);
@@ -105,3 +93,10 @@
int osmo_pfcp_endpoint_tx_heartbeat_req(struct osmo_pfcp_endpoint *ep, const
struct osmo_sockaddr *remote_addr);
void osmo_pfcp_endpoint_invalidate_ctx(struct osmo_pfcp_endpoint *ep, struct
osmo_fsm_inst *deleted_fi);
+
+const struct osmo_pfcp_endpoint_cfg *osmo_pfcp_endpoint_get_cfg(const struct
osmo_pfcp_endpoint *ep);
+void *osmo_pfcp_endpoint_get_priv(const struct osmo_pfcp_endpoint *ep);
+uint32_t osmo_pfcp_endpoint_get_recovery_timestamp(const struct
osmo_pfcp_endpoint *ep);
+void osmo_pfcp_endpoint_set_seq_nr_state(struct osmo_pfcp_endpoint *ep,
uint32_t seq_nr_state);
+
+bool osmo_pfcp_endpoint_retrans_queue_is_busy(const struct osmo_pfcp_endpoint
*ep);
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 @@
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..3002591 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_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 @@
/* 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 @@
/* 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 @@
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 @@
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 @@
/* 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,31 @@
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;
+}
+
+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));
+}
--
To view, visit https://gerrit.osmocom.org/c/libosmo-pfcp/+/28754
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-pfcp
Gerrit-Branch: master
Gerrit-Change-Id: If80c35c6a942bf9593781b5a6bc28ba37323ce5e
Gerrit-Change-Number: 28754
Gerrit-PatchSet: 1
Gerrit-Owner: neels <[email protected]>
Gerrit-MessageType: newchange
