laforge has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-mgw/+/36361?usp=email )
Change subject: Change msgb ownership in processing of received msgb
......................................................................
Change msgb ownership in processing of received msgb
The old approach was: rtp_data_net() reads a msgb from the incomging
socket, calls through whatever function chain and in the end free's it.
So none of the intermediate functions was permitted to take msgb
ownership.
This was a good choice as all processing would happen synchronously,
up to the point where that msgb was written on the output RTP socket.
Let's change this from passing msgb ownership throug the whole call
chain, through rx_rtp() to the various *_dispatch_rtp() functions.
This is required for upcoming migration to osmo_io, as in that case the
write (sendto) calls are asynchronous and hence msgb ownership needs
to be transferred.
Change-Id: I6a331f3c6b2eb51ea312ac6ef8c357185ddb79cf
---
M TODO-RELEASE
M src/libosmo-mgcp/mgcp_e1.c
M src/libosmo-mgcp/mgcp_iuup.c
M src/libosmo-mgcp/mgcp_network.c
M src/libosmo-mgcp/mgcp_osmux.c
5 files changed, 131 insertions(+), 40 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/61/36361/1
diff --git a/TODO-RELEASE b/TODO-RELEASE
index ea29b6a..59da45b 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -39,3 +39,5 @@
is backwards compat code that
moves codecs[] entries, if any, over to
ptmap[], so callers may migrate
at own leisure.
osmo-mgw remove cfg Remove VTY config item 'sdp
audio fmtp-extra' (see OS#6313)
+libosmocore bump_dep; workaround Bump libosmocore version
dependency after I68328adb952ca8833ba047cb3b49ccc6f8a1f1b5
+ has been merged to
libosmocore.git; then remove my_msgb_copy_c wrapper function.
diff --git a/src/libosmo-mgcp/mgcp_e1.c b/src/libosmo-mgcp/mgcp_e1.c
index 40f976b..20aff95 100644
--- a/src/libosmo-mgcp/mgcp_e1.c
+++ b/src/libosmo-mgcp/mgcp_e1.c
@@ -301,7 +301,6 @@
mgcp_send(endp, 1, NULL, msg, &conn_dst->u.rtp, &conn_dst->u.rtp);
- msgb_free(msg);
return;
skip:
rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs,
E1_I460_TRAU_RX_FAIL_CTR));
diff --git a/src/libosmo-mgcp/mgcp_iuup.c b/src/libosmo-mgcp/mgcp_iuup.c
index 6ad62c8..3818d3e 100644
--- a/src/libosmo-mgcp/mgcp_iuup.c
+++ b/src/libosmo-mgcp/mgcp_iuup.c
@@ -311,7 +311,6 @@
};
rc = mgcp_send(conn_rtp_dst->conn->endp, true, NULL, msg, conn_rtp_src,
conn_rtp_dst);
- msgb_free(msg);
return rc;
}
@@ -640,7 +639,7 @@
}
/* Build IuUP RNL Data primitive from msg containing an incoming RTP pkt from
- * peer and send it down the IuUP layer towards the destination as IuUP/RTP: */
+ * peer and send it down the IuUP layer towards the destination as IuUP/RTP.
Takes ownership of msg. */
int mgcp_conn_iuup_send_rtp(struct mgcp_conn_rtp *conn_src_rtp, struct
mgcp_conn_rtp *conn_dest_rtp, struct msgb *msg)
{
struct osmo_iuup_rnl_prim *irp;
diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c
index 0a1bc11..3ed4814 100644
--- a/src/libosmo-mgcp/mgcp_network.c
+++ b/src/libosmo-mgcp/mgcp_network.c
@@ -70,6 +70,18 @@
rtpconn_rate_ctr_add(conn_rtp, endp, id, 1);
}
+/* wrapper around libosmocore msgb_copy_c, which [at least before
libosmocore.git Change-Id
+ * I68328adb952ca8833ba047cb3b49ccc6f8a1f1b5] doesn't copy the cb */
+static inline struct msgb *mgw_msgb_copy_c(void *ctx, struct msgb *msg, const
char *name)
+{
+ struct msgb *msg2 = msgb_copy_c(ctx, msg, name);
+ if (OSMO_UNLIKELY(!msg2))
+ return NULL;
+
+ memcpy(msg2->cb, msg->cb, sizeof(msg2->cb));
+ return msg2;
+}
+
static int rx_rtp(struct msgb *msg);
bool mgcp_rtp_end_remote_addr_available(const struct mgcp_rtp_end *rtp_end)
@@ -963,7 +975,7 @@
return 0;
}
-/*! Dispatch msg bridged from the sister conn in the endpoint.
+/*! Dispatch msg bridged from the sister conn in the endpoint. Takes ownership
of msgb.
* \param[in] conn_dst The destination conn that should handle and transmit
the content to
* its peer outside MGW.
* \param[in] msg msgb containing an RTP pkt received by the sister conn in
the endpoint,
@@ -985,8 +997,10 @@
/* Before we try to deliver the packet, we check if the destination
* port and IP-Address make sense at all. If not, we will be unable
* to deliver the packet. */
- if (check_rtp_destin(conn_dst) != 0)
+ if (check_rtp_destin(conn_dst) != 0) {
+ msgb_free(msg);
return -1;
+ }
/* Depending on the RTP connection type, deliver the RTP packet to the
* destination connection. */
@@ -1021,7 +1035,7 @@
* be discarded, this should not happen, normally the MGCP type
* should be properly set */
LOGPENDP(endp, DRTP, LOGL_ERROR, "bad MGCP type -- data discarded!\n");
-
+ msgb_free(msg);
return -1;
}
@@ -1101,7 +1115,7 @@
return -1;
}
-/*! Send RTP/RTCP data to a specified destination connection.
+/*! Send RTP/RTCP data to a specified destination connection. Takes ownership
of msg.
* \param[in] endp associated endpoint (for configuration, logging).
* \param[in] is_rtp flag to specify if the packet is of type RTP or RTCP.
* \param[in] addr spoofed source address (set to NULL to disable).
@@ -1140,6 +1154,7 @@
if (is_rtp && !mgcp_conn_rtp_is_iuup(conn_src)) {
if (mgcp_patch_pt(conn_dst, msg) < 0) {
LOGPENDP(endp, DRTP, LOGL_NOTICE, "unable to patch
payload type RTP packet, discarding...\n");
+ msgb_free(msg);
return -EINVAL;
}
}
@@ -1212,14 +1227,16 @@
len = mgcp_udp_send(rtp_end->rtp.fd, &rtp_end->addr,
(char *)msgb_data(msg), msgb_length(msg));
-
- if (len <= 0)
+ if (len <= 0) {
+ msgb_free(msg);
return len;
+ }
rtpconn_rate_ctr_inc(conn_dst, endp, RTP_PACKETS_TX_CTR);
rtpconn_rate_ctr_add(conn_dst, endp, RTP_OCTETS_TX_CTR, len);
rtp_state->alt_rtp_tx_sequence++;
+ msgb_free(msg);
return len;
} else if (!trunk->omit_rtcp) {
struct osmo_sockaddr rtcp_addr = rtp_end->addr;
@@ -1238,12 +1255,44 @@
rtpconn_rate_ctr_add(conn_dst, endp, RTP_OCTETS_TX_CTR, len);
rtp_state->alt_rtp_tx_sequence++;
+ msgb_free(msg);
return len;
}
+ msgb_free(msg);
return 0;
}
+/*! determine if there's only a single recipient in endp for data received via
conn_src.
+ * The function returns NULL in case there is no recipient, or in case there
are multiple recipients.
+ * \param endp The MGCP endpoint whose connections to analyze
+ * \param conn_src The source MGCP connection [which shall not count in
results]
+ * \returns recipient donnection if there is only one; NULL in case there are
multiple */
+static struct mgcp_conn *rtpbridge_get_only_recipient(struct mgcp_endpoint
*endp, struct mgcp_conn *conn_src)
+{
+ struct mgcp_conn *conn_ret = NULL;
+ struct mgcp_conn *conn_dst;
+
+ llist_for_each_entry(conn_dst, &endp->conns, entry) {
+ if (conn_dst == conn_src)
+ continue;
+ switch (conn_dst->mode) {
+ case MGCP_CONN_SEND_ONLY:
+ case MGCP_CONN_RECV_SEND:
+ case MGCP_CONN_CONFECHO:
+ if (conn_ret)
+ return NULL;
+ else
+ conn_ret = conn_dst;
+ break;
+ default:
+ break;
+ }
+ }
+
+ return conn_ret;
+}
+
/*! Dispatch incoming RTP packet to opposite RTP connection.
* \param[in] msg Message buffer to bridge, coming from source connection.
* msg shall contain "struct osmo_rtp_msg_ctx *" attached in
@@ -1301,23 +1350,44 @@
return rc;
}
- /* If the mode is "confecho", send RTP back to the sender. */
- if (conn->mode == MGCP_CONN_CONFECHO)
- rc = mgcp_conn_rtp_dispatch_rtp(conn_src, msg);
+ /* All the use cases above are 1:1 where we have one source msgb and
we're sending that to one
+ * destination. msgb ownership had been passed to the respective
_*dospatch_rtp() function.
+ * In the cases below, we actually [can] have multiple recipients, so
we copy the original msgb
+ * for each of the recipients. */
- /* Dispatch RTP packet to all other connection(s) that send audio. */
- llist_for_each_entry(conn_dst, &endp->conns, entry) {
- if (conn_dst == conn)
- continue;
- switch (conn_dst->mode) {
- case MGCP_CONN_SEND_ONLY:
- case MGCP_CONN_RECV_SEND:
- case MGCP_CONN_CONFECHO:
- rc = mgcp_conn_rtp_dispatch_rtp(&conn_dst->u.rtp, msg);
- break;
- default:
- break;
+ /* If the mode is "confecho", send RTP back to the sender. */
+ if (conn->mode == MGCP_CONN_CONFECHO) {
+ struct msgb *msg2 = mgw_msgb_copy_c(conn, msg, "RTP confecho");
+ if (OSMO_LIKELY(msg2))
+ rc = mgcp_conn_rtp_dispatch_rtp(conn_src, msg2);
+ }
+
+ conn_dst = rtpbridge_get_only_recipient(endp, conn);
+ if (OSMO_LIKELY(conn_dst)) {
+ /* we only have a single recipient and cann hence send the
original msgb without copying */
+ rc = mgcp_conn_rtp_dispatch_rtp(&conn_dst->u.rtp, msg);
+ } else {
+ /* Dispatch RTP packet to all other connection(s) that send
audio. */
+ llist_for_each_entry(conn_dst, &endp->conns, entry) {
+ struct msgb *msg2;
+ if (conn_dst == conn)
+ continue;
+ switch (conn_dst->mode) {
+ case MGCP_CONN_SEND_ONLY:
+ case MGCP_CONN_RECV_SEND:
+ case MGCP_CONN_CONFECHO:
+ /* we have multiple recipients and must make
copies for each recipient */
+ msg2 = mgw_msgb_copy_c(conn_dst, msg, "RTP Tx
copy");
+ if (OSMO_LIKELY(msg2))
+ rc =
mgcp_conn_rtp_dispatch_rtp(&conn_dst->u.rtp, msg2);
+ break;
+ default:
+ break;
+ }
}
+ /* as we only sent copies in the previous
llist_for_each_entry() loop, we must free the
+ * original one */
+ msgb_free(msg);
}
return rc;
}
@@ -1471,11 +1541,11 @@
rc = rx_rtp(msg);
out:
- msgb_free(msg);
return rc;
}
-/* Note: This function is able to handle RTP and RTCP */
+/* Note: This function is able to handle RTP and RTCP. msgb ownership is
transferred, so this function or its
+ * downstream consumers must make sure to [eventually] free the msgb. */
static int rx_rtp(struct msgb *msg)
{
struct osmo_rtp_msg_ctx *mc = OSMO_RTP_MSG_CTX(msg);
@@ -1488,7 +1558,7 @@
/* Check if the origin of the RTP packet seems plausible */
if (!trunk->rtp_accept_all && check_rtp_origin(conn_src, from_addr))
- return -1;
+ goto out_free;
/* Handle AMR frame format conversion (octet-aligned vs.
bandwith-efficient) */
if (mc->proto == MGCP_PROTO_RTP
@@ -1498,13 +1568,13 @@
* communicated via SDP when the connection was
created/modfied. */
int oa = amr_oa_check((char*)msgb_data(msg), msgb_length(msg));
if (oa < 0)
- return -1;
+ goto out_free;
if (((bool)oa) != conn_src->end.codec->param.amr_octet_aligned)
{
LOG_CONN_RTP(conn_src, LOGL_NOTICE,
"rx_rtp(%u bytes): Expected RTP AMR
octet-aligned=%u but got octet-aligned=%u."
" check the config of your call-agent!\n",
msgb_length(msg),
conn_src->end.codec->param.amr_octet_aligned, oa);
- return -1;
+ goto out_free;
}
}
@@ -1513,6 +1583,9 @@
/* Execute endpoint specific implementation that handles the
* dispatching of the RTP data */
return conn->endp->type->dispatch_rtp_cb(msg);
+out_free:
+ msgb_free(msg);
+ return -1;
}
/*! bind RTP port to osmo_fd.
diff --git a/src/libosmo-mgcp/mgcp_osmux.c b/src/libosmo-mgcp/mgcp_osmux.c
index 997b07b..70798eb 100644
--- a/src/libosmo-mgcp/mgcp_osmux.c
+++ b/src/libosmo-mgcp/mgcp_osmux.c
@@ -204,14 +204,13 @@
return h->in;
}
-/*! send RTP packet through OSMUX connection.
+/*! send RTP packet through OSMUX connection. Takes ownership of msg.
* \param[in] conn associated RTP connection
* \param[in] msg msgb containing an RTP AMR packet
* \returns 0 on success, -1 on ERROR */
int conn_osmux_send_rtp(struct mgcp_conn_rtp *conn, struct msgb *msg)
{
int ret;
- struct msgb *msg2;
if (!conn->end.output_enabled) {
rtpconn_osmux_rate_ctr_inc(conn,
OSMUX_RTP_PACKETS_TX_DROPPED_CTR);
@@ -225,19 +224,14 @@
return -1;
}
- /* msg is not owned by us and will be freed by the caller stack upon
return: */
- msg2 = msgb_copy_c(conn->conn, msg, "osmux-rtp-send");
- if (!msg2)
- return -1;
-
/* Osmux implementation works with AMR OA only, make sure we convert to
it if needed: */
- if (amr_oa_bwe_convert(conn->conn->endp, msg2, true) < 0) {
+ if (amr_oa_bwe_convert(conn->conn->endp, msg, true) < 0) {
LOGPCONN(conn->conn, DOSMUX, LOGL_ERROR,
"Error converting to AMR octet-aligned mode\n");
return -1;
}
- while ((ret = osmux_xfrm_input(conn->osmux.in, msg2,
conn->osmux.remote_cid)) > 0) {
+ while ((ret = osmux_xfrm_input(conn->osmux.in, msg,
conn->osmux.remote_cid)) > 0) {
/* batch full, build and deliver it */
osmux_xfrm_input_deliver(conn->osmux.in);
}
@@ -245,7 +239,7 @@
rtpconn_osmux_rate_ctr_inc(conn,
OSMUX_RTP_PACKETS_TX_DROPPED_CTR);
} else {
rtpconn_osmux_rate_ctr_inc(conn, OSMUX_RTP_PACKETS_TX_CTR);
- rtpconn_osmux_rate_ctr_add(conn, OSMUX_AMR_OCTETS_TX_CTR,
msgb_length(msg2) - sizeof(struct rtp_hdr));
+ rtpconn_osmux_rate_ctr_add(conn, OSMUX_AMR_OCTETS_TX_CTR,
msgb_length(msg) - sizeof(struct rtp_hdr));
}
return 0;
}
@@ -325,7 +319,7 @@
};
endp->type->dispatch_rtp_cb(msg);
- msgb_free(msg);
+ /* dispatch_rtp_cb() has taken ownership of the msgb */
}
static struct msgb *osmux_recv(struct osmo_fd *ofd, struct osmo_sockaddr *addr)
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/36361?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6a331f3c6b2eb51ea312ac6ef8c357185ddb79cf
Gerrit-Change-Number: 36361
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <lafo...@osmocom.org>
Gerrit-MessageType: newchange
