[PATCH] osmo-mgw[master]: sdp: refactoring sdp parser/generator

dexter gerrit-no-reply at lists.osmocom.org
Wed Oct 11 11:41:54 UTC 2017


Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/4006

to look at the new patch set (#6).

sdp: refactoring sdp parser/generator

move SDP generator function write_response_sdp() from mgcp_protocol.c
to mgcp_sdp.c

move prototypes for mgcp_parse_sdp_data() and mgcp_set_audio_info()
to mgcp_sdp.h

change parameter list of mgcp_parse_sdp_data() so that it takes the
rtp conn directly, rather than struct mgcp_rtp_end.

fix sourcecode formatting in mgcp_sdp.c

add doxygen comments to all public functions

Change-Id: I9f88c93872ff913bc211f560b26901267f577324
---
M include/osmocom/mgcp/Makefile.am
M include/osmocom/mgcp/mgcp_internal.h
A include/osmocom/mgcp/mgcp_sdp.h
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_sdp.c
5 files changed, 211 insertions(+), 142 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/06/4006/6

diff --git a/include/osmocom/mgcp/Makefile.am b/include/osmocom/mgcp/Makefile.am
index 646b887..cd8f599 100644
--- a/include/osmocom/mgcp/Makefile.am
+++ b/include/osmocom/mgcp/Makefile.am
@@ -4,4 +4,5 @@
 	mgcp_conn.h \
 	mgcp_stat.h \
 	mgcp_ep.h \
+	mgcp_sdp.h \
 	$(NULL)
diff --git a/include/osmocom/mgcp/mgcp_internal.h b/include/osmocom/mgcp/mgcp_internal.h
index 3a22d51..777d787 100644
--- a/include/osmocom/mgcp/mgcp_internal.h
+++ b/include/osmocom/mgcp/mgcp_internal.h
@@ -317,9 +317,6 @@
 #define DEFAULT_RTP_AUDIO_DEFAULT_CHANNELS 1
 
 #define PTYPE_UNDEFINED (-1)
-int mgcp_parse_sdp_data(struct mgcp_endpoint *endp, struct mgcp_rtp_end *rtp, struct mgcp_parse_data *p);
-int mgcp_set_audio_info(void *ctx, struct mgcp_rtp_codec *codec,
-			int payload_type, const char *audio_name);
 
 /*! get the ip-address where the mgw application is bound on.
  *  \param[in] endp mgcp endpoint, that holds a copy of the VTY parameters
diff --git a/include/osmocom/mgcp/mgcp_sdp.h b/include/osmocom/mgcp/mgcp_sdp.h
new file mode 100644
index 0000000..0eb376d
--- /dev/null
+++ b/include/osmocom/mgcp/mgcp_sdp.h
@@ -0,0 +1,33 @@
+/*
+ * SDP generation and parsing
+ *
+ * (C) 2009-2015 by Holger Hans Peter Freyther <zecke at selfish.org>
+ * (C) 2009-2014 by On-Waves
+ * All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#pragma once
+
+int mgcp_parse_sdp_data(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn,
+			struct mgcp_parse_data *p);
+
+int mgcp_set_audio_info(void *ctx, struct mgcp_rtp_codec *codec,
+			int payload_type, const char *audio_name);
+
+int mgcp_write_response_sdp(struct mgcp_endpoint *endp,
+			    struct mgcp_conn_rtp *conn, char *sdp_record,
+			    size_t size, const char *addr);
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index ac25894..5d4dd47 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -38,6 +38,7 @@
 #include <osmocom/mgcp/mgcp_stat.h>
 #include <osmocom/mgcp/mgcp_msg.h>
 #include <osmocom/mgcp/mgcp_ep.h>
+#include <osmocom/mgcp/mgcp_sdp.h>
 
 struct mgcp_request {
 	char *name;
@@ -190,80 +191,6 @@
 	return create_resp(endp, code, " FAIL", msg, trans, NULL, NULL);
 }
 
-static int write_response_sdp(struct mgcp_endpoint *endp,
-			      struct mgcp_conn_rtp *conn,
-			      char *sdp_record, size_t size, const char *addr)
-{
-	const char *fmtp_extra;
-	const char *audio_name;
-	int payload_type;
-	int len;
-	int nchars;
-
-	if (!conn)
-		return -1;
-
-	endp->cfg->get_net_downlink_format_cb(endp, &payload_type,
-					      &audio_name, &fmtp_extra, conn);
-
-	len = snprintf(sdp_record, size,
-		       "v=0\r\n"
-		       "o=- %u 23 IN IP4 %s\r\n"
-		       "s=-\r\n"
-		       "c=IN IP4 %s\r\n"
-		       "t=0 0\r\n", conn->conn->id, addr, addr);
-
-	if (len < 0 || len >= size)
-		goto buffer_too_small;
-
-	if (payload_type >= 0) {
-		nchars = snprintf(sdp_record + len, size - len,
-				  "m=audio %d RTP/AVP %d\r\n",
-				  conn->end.local_port, payload_type);
-		if (nchars < 0 || nchars >= size - len)
-			goto buffer_too_small;
-
-		len += nchars;
-
-		if (audio_name && endp->tcfg->audio_send_name) {
-			nchars = snprintf(sdp_record + len, size - len,
-					  "a=rtpmap:%d %s\r\n",
-					  payload_type, audio_name);
-
-			if (nchars < 0 || nchars >= size - len)
-				goto buffer_too_small;
-
-			len += nchars;
-		}
-
-		if (fmtp_extra) {
-			nchars = snprintf(sdp_record + len, size - len,
-					  "%s\r\n", fmtp_extra);
-
-			if (nchars < 0 || nchars >= size - len)
-				goto buffer_too_small;
-
-			len += nchars;
-		}
-	}
-	if (conn->end.packet_duration_ms > 0 && endp->tcfg->audio_send_ptime) {
-		nchars = snprintf(sdp_record + len, size - len,
-				  "a=ptime:%u\r\n",
-				  conn->end.packet_duration_ms);
-		if (nchars < 0 || nchars >= size - len)
-			goto buffer_too_small;
-
-		len += nchars;
-	}
-
-	return len;
-
-buffer_too_small:
-	LOGP(DLMGCP, LOGL_ERROR, "SDP buffer too small: %zu (needed %d)\n",
-	     size, len);
-	return -1;
-}
-
 /* Format MGCP response string (with SDP attached) */
 static struct msgb *create_response_with_sdp(struct mgcp_endpoint *endp,
 					     struct mgcp_conn_rtp *conn,
@@ -291,8 +218,8 @@
 	if (len < 0)
 		return NULL;
 
-	nchars = write_response_sdp(endp, conn, sdp_record + len,
-				    sizeof(sdp_record) - len - 1, addr);
+	nchars = mgcp_write_response_sdp(endp, conn, sdp_record + len,
+					 sizeof(sdp_record) - len - 1, addr);
 	if (nchars < 0)
 		return NULL;
 
@@ -688,7 +615,7 @@
 
 	/* set up RTP media parameters */
 	if (have_sdp)
-		mgcp_parse_sdp_data(endp, &conn->end, p);
+		mgcp_parse_sdp_data(endp, conn, p);
 	else if (endp->local_options.codec)
 		mgcp_set_audio_info(p->cfg, &conn->end.codec,
 				    PTYPE_UNDEFINED, endp->local_options.codec);
@@ -835,7 +762,7 @@
 			conn->conn->mode = conn->conn->mode_orig;
 
 	if (have_sdp)
-		mgcp_parse_sdp_data(endp, &conn->end, p);
+		mgcp_parse_sdp_data(endp, conn, p);
 
 	set_local_cx_options(endp->tcfg->endpoints, &endp->local_options,
 			     local_options);
diff --git a/src/libosmo-mgcp/mgcp_sdp.c b/src/libosmo-mgcp/mgcp_sdp.c
index 7568351..048a533 100644
--- a/src/libosmo-mgcp/mgcp_sdp.c
+++ b/src/libosmo-mgcp/mgcp_sdp.c
@@ -38,6 +38,12 @@
 	int channels;
 };
 
+/*! set codec configuration depending on payload type and codec name.
+ *  \endp[in] ctx talloc context
+ *  \endp[out] codec configuration (caller provided memory)
+ *  \endp[in] payload_type codec type id (e.g. 3 for GSM, -1 when undefined)
+ *  \endp[in] audio_name audio codec name (e.g. "GSM/8000/1")
+ *  \returns 0 on success, -1 on failure */
 int mgcp_set_audio_info(void *ctx, struct mgcp_rtp_codec *codec,
 			int payload_type, const char *audio_name)
 {
@@ -55,15 +61,23 @@
 
 	if (!audio_name) {
 		switch (payload_type) {
-		case 0: audio_name = "PCMU/8000/1"; break;
-		case 3: audio_name = "GSM/8000/1"; break;
-		case 8: audio_name = "PCMA/8000/1"; break;
-		case 18: audio_name = "G729/8000/1"; break;
+		case 0:
+			audio_name = "PCMU/8000/1";
+			break;
+		case 3:
+			audio_name = "GSM/8000/1";
+			break;
+		case 8:
+			audio_name = "PCMA/8000/1";
+			break;
+		case 18:
+			audio_name = "G729/8000/1";
+			break;
 		default:
-			 /* Payload type is unknown, don't change rate and
-			  * channels. */
-			 /* TODO: return value? */
-			 return 0;
+			/* Payload type is unknown, don't change rate and
+			 * channels. */
+			/* TODO: return value? */
+			return 0;
 		}
 	}
 
@@ -107,7 +121,7 @@
 	return 0;
 }
 
-void codecs_initialize(void *ctx, struct sdp_rtp_map *codecs, int used)
+static void codecs_initialize(void *ctx, struct sdp_rtp_map *codecs, int used)
 {
 	int i;
 
@@ -137,7 +151,8 @@
 	}
 }
 
-void codecs_update(void *ctx, struct sdp_rtp_map *codecs, int used, int payload, char *audio_name)
+static void codecs_update(void *ctx, struct sdp_rtp_map *codecs, int used,
+			  int payload, char *audio_name)
 {
 	int i;
 
@@ -148,8 +163,9 @@
 		if (codecs[i].payload_type != payload)
 			continue;
 		if (sscanf(audio_name, "%63[^/]/%d/%d",
-				audio_codec, &rate, &channels) < 1) {
-			LOGP(DLMGCP, LOGL_ERROR, "Failed to parse '%s'\n", audio_name);
+			   audio_codec, &rate, &channels) < 1) {
+			LOGP(DLMGCP, LOGL_ERROR, "Failed to parse '%s'\n",
+			     audio_name);
 			continue;
 		}
 
@@ -160,29 +176,36 @@
 		return;
 	}
 
-	LOGP(DLMGCP, LOGL_ERROR, "Unconfigured PT(%d) with %s\n", payload, audio_name);
+	LOGP(DLMGCP, LOGL_ERROR, "Unconfigured PT(%d) with %s\n", payload,
+	     audio_name);
 }
 
-int is_codec_compatible(struct mgcp_endpoint *endp, struct sdp_rtp_map *codec)
+/* Check if the codec matches what is set up in the trunk config */
+static int is_codec_compatible(struct mgcp_endpoint *endp,
+			       struct sdp_rtp_map *codec)
 {
-	char *bts_codec;
+	char *codec_str;
 	char audio_codec[64];
 
 	if (!codec->codec_name)
 		return 0;
 
-	/*
-	 * GSM, GSM/8000 and GSM/8000/1 should all be compatible.. let's go
-	 * by name first.
-	 */
-	bts_codec = endp->tcfg->audio_name;
-	if (sscanf(bts_codec, "%63[^/]/%*d/%*d", audio_codec) < 1)
+	/* GSM, GSM/8000 and GSM/8000/1 should all be compatible...
+	 * let's go by name first. */
+	codec_str = endp->tcfg->audio_name;
+	if (sscanf(codec_str, "%63[^/]/%*d/%*d", audio_codec) < 1)
 		return 0;
 
 	return strcasecmp(audio_codec, codec->codec_name) == 0;
 }
 
-int mgcp_parse_sdp_data(struct mgcp_endpoint *endp, struct mgcp_rtp_end *rtp, struct mgcp_parse_data *p)
+/*! analyze SDP input string.
+ *  \endp[in] endp trunk endpoint
+ *  \endp[in] conn associated rtp connection
+ *  \endp[in] caller provided memory to store the parsing results
+ *  \returns 0 on success, -1 on failure */
+int mgcp_parse_sdp_data(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn,
+			struct mgcp_parse_data *p)
 {
 	struct sdp_rtp_map codecs[10];
 	int codecs_used = 0;
@@ -191,7 +214,19 @@
 	int i;
 	int codecs_assigned = 0;
 	void *tmp_ctx = talloc_new(NULL);
+	struct mgcp_rtp_end *rtp;
 
+	int payload;
+	int ptime, ptime2 = 0;
+	char audio_name[64];
+	int port, rc;
+	char ipv4[16];
+
+	OSMO_ASSERT(endp);
+	OSMO_ASSERT(conn);
+	OSMO_ASSERT(p);
+
+	rtp = &conn->end;
 	memset(&codecs, 0, sizeof(codecs));
 
 	for_each_line(line, p->save) {
@@ -202,41 +237,36 @@
 		case 'v':
 			/* skip these SDP attributes */
 			break;
-		case 'a': {
-			int payload;
-			int ptime, ptime2 = 0;
-			char audio_name[64];
-
-
+		case 'a':
 			if (sscanf(line, "a=rtpmap:%d %63s",
 				   &payload, audio_name) == 2) {
-				codecs_update(tmp_ctx, codecs, codecs_used, payload, audio_name);
-			} else if (sscanf(line, "a=ptime:%d-%d",
-					  &ptime, &ptime2) >= 1) {
+				codecs_update(tmp_ctx, codecs,
+					      codecs_used, payload, audio_name);
+			} else
+			    if (sscanf
+				(line, "a=ptime:%d-%d", &ptime, &ptime2) >= 1) {
 				if (ptime2 > 0 && ptime2 != ptime)
 					rtp->packet_duration_ms = 0;
 				else
 					rtp->packet_duration_ms = ptime;
-			} else if (sscanf(line, "a=maxptime:%d", &ptime2) == 1) {
+			} else if (sscanf(line, "a=maxptime:%d", &ptime2)
+				   == 1) {
 				maxptime = ptime2;
 			}
 			break;
-		}
-		case 'm': {
-			int port, rc;
-
-			rc = sscanf(line, "m=audio %d RTP/AVP %d %d %d %d %d %d %d %d %d %d",
-					&port,
-					&codecs[0].payload_type,
-					&codecs[1].payload_type,
-					&codecs[2].payload_type,
-					&codecs[3].payload_type,
-					&codecs[4].payload_type,
-					&codecs[5].payload_type,
-					&codecs[6].payload_type,
-					&codecs[7].payload_type,
-					&codecs[8].payload_type,
-					&codecs[9].payload_type);
+		case 'm':
+			rc = sscanf(line,
+				    "m=audio %d RTP/AVP %d %d %d %d %d %d %d %d %d %d",
+				    &port, &codecs[0].payload_type,
+				    &codecs[1].payload_type,
+				    &codecs[2].payload_type,
+				    &codecs[3].payload_type,
+				    &codecs[4].payload_type,
+				    &codecs[5].payload_type,
+				    &codecs[6].payload_type,
+				    &codecs[7].payload_type,
+				    &codecs[8].payload_type,
+				    &codecs[9].payload_type);
 			if (rc >= 2) {
 				rtp->rtp_port = htons(port);
 				rtp->rtcp_port = htons(port + 1);
@@ -244,20 +274,18 @@
 				codecs_initialize(tmp_ctx, codecs, codecs_used);
 			}
 			break;
-		}
-		case 'c': {
-			char ipv4[16];
+		case 'c':
 
 			if (sscanf(line, "c=IN IP4 %15s", ipv4) == 1) {
 				inet_aton(ipv4, &rtp->addr);
 			}
 			break;
-		}
 		default:
 			if (p->endp)
 				LOGP(DLMGCP, LOGL_NOTICE,
 				     "Unhandled SDP option: '%c'/%d on 0x%x\n",
-				     line[0], line[0], ENDPOINT_NUMBER(p->endp));
+				     line[0], line[0],
+				     ENDPOINT_NUMBER(p->endp));
 			else
 				LOGP(DLMGCP, LOGL_NOTICE,
 				     "Unhandled SDP option: '%c'/%d\n",
@@ -269,25 +297,24 @@
 	/* Now select the primary and alt_codec */
 	for (i = 0; i < codecs_used && codecs_assigned < 2; ++i) {
 		struct mgcp_rtp_codec *codec = codecs_assigned == 0 ?
-					&rtp->codec : &rtp->alt_codec;
+		    &rtp->codec : &rtp->alt_codec;
 
 		if (endp->tcfg->no_audio_transcoding &&
-			!is_codec_compatible(endp, &codecs[i])) {
+		    !is_codec_compatible(endp, &codecs[i])) {
 			LOGP(DLMGCP, LOGL_NOTICE, "Skipping codec %s\n",
-				codecs[i].codec_name);
+			     codecs[i].codec_name);
 			continue;
 		}
 
 		mgcp_set_audio_info(p->cfg, codec,
-					codecs[i].payload_type,
-					codecs[i].map_line);
+				    codecs[i].payload_type, codecs[i].map_line);
 		codecs_assigned += 1;
 	}
 
 	if (codecs_assigned > 0) {
 		/* TODO/XXX: Store this per codec and derive it on use */
 		if (maxptime >= 0 && maxptime * rtp->codec.frame_duration_den >
-				rtp->codec.frame_duration_num * 1500) {
+		    rtp->codec.frame_duration_num * 1500) {
 			/* more than 1 frame */
 			rtp->packet_duration_ms = 0;
 		}
@@ -296,11 +323,95 @@
 		     "Got media info via SDP: port %d, payload %d (%s), "
 		     "duration %d, addr %s\n",
 		     ntohs(rtp->rtp_port), rtp->codec.payload_type,
-		     rtp->codec.subtype_name ? rtp->codec.subtype_name : "unknown",
-		     rtp->packet_duration_ms, inet_ntoa(rtp->addr));
+		     rtp->codec.subtype_name ? rtp->
+		     codec.subtype_name : "unknown", rtp->packet_duration_ms,
+		     inet_ntoa(rtp->addr));
 	}
 
 	talloc_free(tmp_ctx);
 	return codecs_assigned > 0;
 }
 
+/*! generate SDP response string.
+ *  \endp[in] endp trunk endpoint
+ *  \endp[in] conn associated rtp connection
+ *  \endp[out] sdp_record resulting SDP string
+ *  \endp[in] size buffer size of sdp_record
+ *  \endp[in] addr IPV4 address string (e.g. 192.168.100.1)
+ *  \returns 0 on success, -1 on failure */
+int mgcp_write_response_sdp(struct mgcp_endpoint *endp,
+			    struct mgcp_conn_rtp *conn, char *sdp_record,
+			    size_t size, const char *addr)
+{
+	const char *fmtp_extra;
+	const char *audio_name;
+	int payload_type;
+	int len;
+	int nchars;
+
+	OSMO_ASSERT(endp);
+	OSMO_ASSERT(conn);
+	OSMO_ASSERT(sdp_record);
+	OSMO_ASSERT(size > 0);
+	OSMO_ASSERT(addr);
+
+	endp->cfg->get_net_downlink_format_cb(endp, &payload_type,
+					      &audio_name, &fmtp_extra, conn);
+
+	len = snprintf(sdp_record, size,
+		       "v=0\r\n"
+		       "o=- %u 23 IN IP4 %s\r\n"
+		       "s=-\r\n"
+		       "c=IN IP4 %s\r\n"
+		       "t=0 0\r\n", conn->conn->id, addr, addr);
+
+	if (len < 0 || len >= size)
+		goto buffer_too_small;
+
+	if (payload_type >= 0) {
+		nchars = snprintf(sdp_record + len, size - len,
+				  "m=audio %d RTP/AVP %d\r\n",
+				  conn->end.local_port, payload_type);
+		if (nchars < 0 || nchars >= size - len)
+			goto buffer_too_small;
+
+		len += nchars;
+
+		if (audio_name && endp->tcfg->audio_send_name) {
+			nchars = snprintf(sdp_record + len, size - len,
+					  "a=rtpmap:%d %s\r\n",
+					  payload_type, audio_name);
+
+			if (nchars < 0 || nchars >= size - len)
+				goto buffer_too_small;
+
+			len += nchars;
+		}
+
+		if (fmtp_extra) {
+			nchars = snprintf(sdp_record + len, size - len,
+					  "%s\r\n", fmtp_extra);
+
+			if (nchars < 0 || nchars >= size - len)
+				goto buffer_too_small;
+
+			len += nchars;
+		}
+	}
+	if (conn->end.packet_duration_ms > 0 && endp->tcfg->audio_send_ptime) {
+		nchars = snprintf(sdp_record + len, size - len,
+				  "a=ptime:%u\r\n",
+				  conn->end.packet_duration_ms);
+		if (nchars < 0 || nchars >= size - len)
+			goto buffer_too_small;
+
+		len += nchars;
+	}
+
+	return len;
+
+buffer_too_small:
+	LOGP(DLMGCP, LOGL_ERROR, "SDP buffer too small: %zu (needed %d)\n",
+	     size, len);
+	return -1;
+}

-- 
To view, visit https://gerrit.osmocom.org/4006
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f88c93872ff913bc211f560b26901267f577324
Gerrit-PatchSet: 6
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>


More information about the gerrit-log mailing list