[M] Change in osmo-mgw[master]: mgw: Decouple SDP parsing step from conn obj update - gerrit-log

7 Jan 2025

pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-mgw/+/39248?usp=email )
Change subject: mgw: Decouple SDP parsing step from conn obj update
......................................................................
mgw: Decouple SDP parsing step from conn obj update
This allows delaying modification of the conn before full parsing
succeeds.
Change-Id: I4a2aecb542886672c1f586c6607714e0356abe35
---
M include/osmocom/mgcp/mgcp_protocol.h
M include/osmocom/mgcp/mgcp_sdp.h
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_sdp.c
4 files changed, 118 insertions(+), 69 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/48/39248/1

diff --git a/include/osmocom/mgcp/mgcp_protocol.h b/include/osmocom/mgcp/mgcp_protocol.h
index ffe7ae0..a6a7cec 100644
--- a/include/osmocom/mgcp/mgcp_protocol.h
+++ b/include/osmocom/mgcp/mgcp_protocol.h
@@ -1,7 +1,35 @@
 #pragma once
+#include <stdint.h>
+#include <sys/socket.h>
+
 #include <osmocom/core/utils.h>
+#include <osmocom/core/socket.h>
 #include <osmocom/mgcp/mgcp_common.h>
+#include <osmocom/mgcp/mgcp_codec.h>
+
+
+#define MGCP_PARSE_SDP_PTIME_UNSET (-1)
+#define MGCP_PARSE_SDP_MAXPTIME_UNSET (-1)
+#define MGCP_PARSE_SDP_RTP_PORT_UNSET (0)
+
+struct mgcp_parse_sdp {
+	int ptime;
+	int maxptime;
+	int rtp_port;
+	struct osmo_sockaddr rem_addr; /* Only IP address, port is in rtp_port above */
+	struct mgcp_rtp_codecset cset;
+};
+
+static inline void mgcp_parse_sdp_init(struct mgcp_parse_sdp *sdp)
+{
+	sdp->ptime = MGCP_PARSE_SDP_PTIME_UNSET;
+	sdp->maxptime = MGCP_PARSE_SDP_MAXPTIME_UNSET;
+	sdp->rtp_port = MGCP_PARSE_SDP_RTP_PORT_UNSET;
+	sdp->rem_addr = (struct osmo_sockaddr){ .u.sa.sa_family = AF_UNSPEC };
+	mgcp_codecset_reset(&sdp->cset);
+}
+
#define MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET (-2)
 #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1)
@@ -39,6 +67,7 @@
    char *trans;
    /* MGCP Body: */
    struct mgcp_parse_hdr_pars hpars;
+	struct mgcp_parse_sdp sdp;
 };
/* Local connection options */
diff --git a/include/osmocom/mgcp/mgcp_sdp.h b/include/osmocom/mgcp/mgcp_sdp.h
index 950092e..4a1313f 100644
--- a/include/osmocom/mgcp/mgcp_sdp.h
+++ b/include/osmocom/mgcp/mgcp_sdp.h
@@ -22,9 +22,9 @@
#pragma once
-int mgcp_parse_sdp_data(const struct mgcp_endpoint *endp,
-			struct mgcp_conn_rtp *conn,
-			struct mgcp_parse_data *p);
+struct mgcp_parse_data;
+
+int mgcp_parse_sdp_data(struct mgcp_parse_data *p);
int mgcp_write_response_sdp(const struct mgcp_endpoint *endp,
    		    const struct mgcp_conn_rtp *conn, struct msgb *sdp,
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index d066f29..dfd3e0c 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -716,36 +716,59 @@
    return 0;
 }
+/* Apply parsed SDP information stored in struct mgcp_parse_sdp to conn_rtp: */
+static int handle_sdp(struct mgcp_conn_rtp *conn, struct mgcp_request_data *rq)
+{
+	OSMO_ASSERT(conn);
+	OSMO_ASSERT(rq);
+	struct mgcp_parse_data *p = rq->pdata;
+	OSMO_ASSERT(p);
+	OSMO_ASSERT(p->hpars.have_sdp);
+	struct mgcp_parse_sdp *sdp = &p->sdp;
+	struct mgcp_rtp_end *rtp;
+
+	rtp = &conn->end;
+
+	if (sdp->ptime != MGCP_PARSE_SDP_PTIME_UNSET)
+		mgcp_rtp_end_set_packet_duration_ms(rtp, sdp->ptime);
+
+	if (sdp->maxptime != MGCP_PARSE_SDP_MAXPTIME_UNSET)
+		rtp->maximum_packet_time = sdp->maxptime;
+
+	if (sdp->rem_addr.u.sa.sa_family != AF_UNSPEC) {
+		/* Keep port, only apply ip address: */
+		uint16_t port = osmo_sockaddr_port(&rtp->addr.u.sa);
+		memcpy(&rtp->addr, &sdp->rem_addr, sizeof(rtp->addr));
+		osmo_sockaddr_set_port(&rtp->addr.u.sa, port);
+	}
+
+	if (sdp->rtp_port != MGCP_PARSE_SDP_RTP_PORT_UNSET) {
+		osmo_sockaddr_set_port(&rtp->addr.u.sa, sdp->rtp_port);
+		rtp->rtcp_port = htons(sdp->rtp_port + 1);
+	}
+
+	/* Copy parsed codec set to conn: */
+	rtp->cset = sdp->cset;
+
+	return 0;
+}
+
 /* Process codec information contained in CRCX/MDCX */
-static int handle_codec_info(struct mgcp_conn_rtp *conn,
-			     struct mgcp_request_data *rq, int have_sdp, bool crcx)
+static int handle_codec_info(struct mgcp_conn_rtp *conn, struct mgcp_request_data *rq)
 {
    struct mgcp_endpoint *endp = rq->endp;
    struct mgcp_conn *conn_dst;
    struct mgcp_conn_rtp *conn_dst_rtp;
    struct mgcp_rtp_codecset *cset = &conn->end.cset;
-
    int rc;
-	char *cmd;
-
-	if (crcx)
-		cmd = "CRCX";
-	else
-		cmd = "MDCX";
/* Collect codec information */
-	if (have_sdp) {
+	if (rq->pdata->hpars.have_sdp) {
    	/* If we have SDP, we ignore the local connection options and
    	 * use only the SDP information. */
-		mgcp_codecset_reset(cset);
-		rc = mgcp_parse_sdp_data(endp, conn, rq->pdata);
-		if (rc != 0) {
-			LOGPCONN(conn->conn, DLMGCP,  LOGL_ERROR,
-				 "%s: sdp not parseable\n", cmd);
-
-			/* See also RFC 3661: Protocol error */
-			return 510;
-		}
+		rc = handle_sdp(conn, rq);
+		if (rc != 0)
+			goto error;
    } else if (endp->local_options.codec) {
    	/* When no SDP is available, we use the codec information from
    	 * the local connection options (if present) */
@@ -780,9 +803,7 @@
    return 0;
error:
-	LOGPCONN(conn->conn, DLMGCP, LOGL_ERROR,
-	     "%s: codec negotiation failure\n", cmd);
-
+	LOGPCONN(conn->conn, DLMGCP, LOGL_ERROR, "%s: codec negotiation failure\n", rq->name);
    /* See also RFC 3661: Codec negotiation failure */
    return 534;
 }
@@ -861,6 +882,15 @@
    	return create_err_response(endp, endp, 523, "CRCX", pdata->trans);
    }
+	/* Parse SDP if found: */
+	if (hpars->have_sdp) {
+		rc = mgcp_parse_sdp_data(pdata);
+		if (rc < 0) { /* See also RFC 3661: Protocol error */
+			LOGPENDP(endp, DLMGCP,  LOGL_ERROR, "CRCX: sdp not parseable\n");
+			return create_err_response(endp, endp, 510, "CRCX", pdata->trans);
+		}
+	}
+
    /* If osmux is disabled, just skip setting it up */
    if (trunk->cfg->osmux.usage == OSMUX_USAGE_OFF)
    	hpars->remote_osmux_cid = MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET;
@@ -969,7 +999,7 @@
    }
/* Handle codec information and decide for a suitable codec */
-	rc = handle_codec_info(conn_rtp, rq, hpars->have_sdp, true);
+	rc = handle_codec_info(conn_rtp, rq);
    mgcp_codecset_summary(&conn_rtp->end.cset, mgcp_conn_dump(conn));
    if (rc) {
    	error_code = rc;
@@ -1103,6 +1133,15 @@
    	return create_err_response(endp, endp, error_code, "MDCX", pdata->trans);
    }
+	/* Parse SDP if found: */
+	if (hpars->have_sdp) {
+		rc = mgcp_parse_sdp_data(pdata);
+		if (rc < 0) { /* See also RFC 3661: Protocol error */
+			LOGPENDP(endp, DLMGCP,  LOGL_ERROR, "MDCX: sdp not parseable\n");
+			return create_err_response(endp, endp, 510, "MDCX", pdata->trans);
+		}
+	}
+
    conn = mgcp_endp_get_conn(endp, hpars->connid);
    if (!conn) {
    	rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_MDCX_FAIL_CONN_NOT_FOUND));
@@ -1136,7 +1175,7 @@
    OSMO_ASSERT(conn_rtp);
/* Handle codec information and decide for a suitable codec */
-	rc = handle_codec_info(conn_rtp, rq, hpars->have_sdp, false);
+	rc = handle_codec_info(conn_rtp, rq);
    mgcp_codecset_summary(&conn_rtp->end.cset, mgcp_conn_dump(conn));
    if (rc) {
    	error_code = rc;
diff --git a/src/libosmo-mgcp/mgcp_sdp.c b/src/libosmo-mgcp/mgcp_sdp.c
index ffd0f18..1d76ddb 100644
--- a/src/libosmo-mgcp/mgcp_sdp.c
+++ b/src/libosmo-mgcp/mgcp_sdp.c
@@ -312,16 +312,13 @@
 }
/*! Analyze SDP input string.
- *  \param[in] endp trunk endpoint.
- *  \param[out] conn associated rtp connection.
- *  \param[out] caller provided memory to store the parsing results.
+ *  \param[inout] p provided memory to store the parsing results.
  *
- *  Note: In conn (conn->end) the function returns the packet duration,
- *  rtp port, rtcp port and the codec information.
  *  \returns 0 on success, -1 on failure. */
-int mgcp_parse_sdp_data(const struct mgcp_endpoint *endp,
-			struct mgcp_conn_rtp *conn, struct mgcp_parse_data *p)
+int mgcp_parse_sdp_data(struct mgcp_parse_data *p)
 {
+	OSMO_ASSERT(p);
+	struct mgcp_parse_sdp *sdp = &p->sdp;
    struct sdp_rtp_map codecs[MGCP_MAX_CODECS];
    unsigned int codecs_used = 0;
    struct sdp_fmtp_param fmtp_params[MGCP_MAX_CODECS];
@@ -331,19 +328,14 @@
    char *line;
    unsigned int i;
    void *tmp_ctx = talloc_new(NULL);
-	struct mgcp_rtp_end *rtp;
int payload_type;
    int ptime, ptime2 = 0;
    char audio_name[64];
    int port, rc;
-	OSMO_ASSERT(endp);
-	OSMO_ASSERT(conn);
-	OSMO_ASSERT(p);
-
-	rtp = &conn->end;
    memset(&codecs, 0, sizeof(codecs));
+	mgcp_parse_sdp_init(sdp);
for_each_line(line, p->save) {
    	switch (line[0]) {
@@ -361,19 +353,19 @@
if (sscanf(line, "a=ptime:%d-%d", &ptime, &ptime2) >= 1) {
    			if (ptime2 > 0 && ptime2 != ptime)
-					mgcp_rtp_end_set_packet_duration_ms(rtp, 0);
+					sdp->ptime = 0;
    			else
-					mgcp_rtp_end_set_packet_duration_ms(rtp, ptime);
+					sdp->ptime = ptime;
    			break;
    		}
if (sscanf(line, "a=maxptime:%d", &ptime2) == 1) {
-				rtp->maximum_packet_time = ptime2;
+				sdp->maxptime = ptime2;
    			break;
    		}
if (strncmp("a=fmtp:", line, 6) == 0) {
-				rc = fmtp_from_sdp(conn->conn, &fmtp_params[fmtp_used], line);
+				rc = fmtp_from_sdp(tmp_ctx, &fmtp_params[fmtp_used], line);
    			if (rc >= 0)
    				fmtp_used++;
    			break;
@@ -382,33 +374,23 @@
    		break;
    	case 'm':
    		rc = sscanf(line, "m=audio %d RTP/AVP", &port);
-			if (rc == 1) {
-				osmo_sockaddr_set_port(&rtp->addr.u.sa, port);
-				rtp->rtcp_port = htons(port + 1);
-			}
+			if (rc == 1)
+				sdp->rtp_port = port;
-			rc = pt_from_sdp(conn->conn, codecs,
-					 ARRAY_SIZE(codecs), line);
+			rc = pt_from_sdp(tmp_ctx, codecs, ARRAY_SIZE(codecs), line);
    		if (rc > 0)
    			codecs_used = rc;
    		break;
    	case 'c':
-			if (audio_ip_from_sdp(&rtp->addr, line) < 0) {
+			if (audio_ip_from_sdp(&sdp->rem_addr, line) < 0) {
    			talloc_free(tmp_ctx);
    			return -1;
    		}
    		break;
    	default:
-			if (endp)
-				/* TODO: Check spec: We used the bare endpoint number before,
-				 * now we use the endpoint name as a whole? Is this allowed? */
-				LOGP(DLMGCP, LOGL_NOTICE,
-				     "Unhandled SDP option: '%c'/%d on %s\n",
-				     line[0], line[0], endp->name);
-			else
-				LOGP(DLMGCP, LOGL_NOTICE,
-				     "Unhandled SDP option: '%c'/%d\n",
-				     line[0], line[0]);
+			LOGP(DLMGCP, LOGL_NOTICE,
+			     "Unhandled SDP option: '%c'/%d on %s\n",
+			     line[0], line[0], p->epname);
    		break;
    	}
    }
@@ -422,23 +404,22 @@
    /* Store parsed codec information */
    for (i = 0; i < codecs_used; i++) {
    	codec_param = param_by_pt(codecs[i].payload_type, fmtp_params, fmtp_used);
-		rc = mgcp_codecset_add_codec(&conn->end.cset, codecs[i].payload_type, codecs[i].map_line, codec_param);
+		rc = mgcp_codecset_add_codec(&sdp->cset, codecs[i].payload_type, codecs[i].map_line, codec_param);
    	if (rc < 0)
-			LOGPENDP(endp, DLMGCP, LOGL_NOTICE, "failed to add codec\n");
+			LOGP(DLMGCP, LOGL_NOTICE, "%s: failed to add codec\n", p->epname);
    }
talloc_free(tmp_ctx);
-	LOGPCONN(conn->conn, DLMGCP, LOGL_NOTICE,
-	     "Got media info via SDP: port:%d, addr:%s, duration:%d, payload-types:",
-	     osmo_sockaddr_port(&rtp->addr.u.sa), osmo_sockaddr_ntop(&rtp->addr.u.sa, ipbuf),
-	     rtp->packet_duration_ms);
+	LOGP(DLMGCP, LOGL_NOTICE,
+	     "%s: Got media info via SDP: port:%d, addr:%s, duration:%d, payload-types:",
+	     p->epname, sdp->rtp_port, osmo_sockaddr_ntop(&sdp->rem_addr.u.sa, ipbuf), sdp->ptime);
    if (codecs_used == 0)
    	LOGPC(DLMGCP, LOGL_NOTICE, "none");
    for (i = 0; i < codecs_used; i++) {
    	LOGPC(DLMGCP, LOGL_NOTICE, "%d=%s",
-		      rtp->cset.codecs[i].payload_type,
-		      strlen(rtp->cset.codecs[i].subtype_name) ? rtp->cset.codecs[i].subtype_name : "unknown");
+		      sdp->cset.codecs[i].payload_type,
+		      strlen(sdp->cset.codecs[i].subtype_name) ? sdp->cset.codecs[i].subtype_name : "unknown");
    	LOGPC(DLMGCP, LOGL_NOTICE, " ");
    }
    LOGPC(DLMGCP, LOGL_NOTICE, "\n");
-- 
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39248?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I4a2aecb542886672c1f586c6607714e0356abe35
Gerrit-Change-Number: 39248
Gerrit-PatchSet: 1
Gerrit-Owner: pespin pespin@sysmocom.de



    