Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors

[neels]

neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15138


Change subject: mgcp_codec: codec_set(): log about all possible errors
......................................................................

mgcp_codec: codec_set(): log about all possible errors

In codec_set(), for each 'goto error', log the specific error cause.

Also add a TODO and a FIXME comment about inventing dynamic payload type
numbers.

Change-Id: I0b44b574c814882b6f8ae7cd738a6f481cd721fd
---
M src/libosmo-mgcp/mgcp_codec.c
1 file changed, 27 insertions(+), 9 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/38/15138/1

diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c
index a947bb3..93a8157 100644
--- a/src/libosmo-mgcp/mgcp_codec.c
+++ b/src/libosmo-mgcp/mgcp_codec.c
@@ -121,12 +121,13 @@
 	if (payload_type != PTYPE_UNDEFINED) {
 		/* Make sure we do not get any reserved or undefined type numbers */
 		/* See also: https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml */
-		if (payload_type == 1 || payload_type == 2 || payload_type == 19)
+		if ((payload_type == 1 || payload_type == 2 || payload_type == 19)
+		    || (payload_type >= 72 && payload_type <= 76)
+		    || (payload_type >= 127)) {
+			LOGP(DLMGCP, LOGL_ERROR, "Cannot add codec, payload type number %d is reserved\n",
+			     payload_type);
 			goto error;
-		if (payload_type >= 72 && payload_type <= 76)
-			goto error;
-		if (payload_type >= 127)
-			goto error;
+		}
 
 		codec->payload_type = payload_type;
 	}
@@ -152,6 +153,8 @@
 			/* The given payload type is not known to us, or it
 			 * it is a dynamic payload type for which we do not
 			 * know the audio name. We must give up here */
+			LOGP(DLMGCP, LOGL_ERROR, "No audio codec name given, and payload type %d unknown\n",
+			     payload_type);
 			goto error;
 		}
 	}
@@ -159,16 +162,23 @@
 	/* Now we extract the codec subtype name, rate and channels. The latter
 	 * two are optional. If they are not present we use the safe defaults
 	 * above. */
-	if (strlen(audio_name) > sizeof(audio_codec))
+	if (strlen(audio_name) > sizeof(audio_codec)) {
+		LOGP(DLMGCP, LOGL_ERROR, "Audio codec too long: %s\n", osmo_quote_str(audio_name, -1));
 		goto error;
+	}
 	channels = DEFAULT_RTP_AUDIO_DEFAULT_CHANNELS;
 	rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE;
-	if (sscanf(audio_name, "%63[^/]/%d/%d", audio_codec, &rate, &channels) < 1)
+	if (sscanf(audio_name, "%63[^/]/%d/%d", audio_codec, &rate, &channels) < 1) {
+		LOGP(DLMGCP, LOGL_ERROR, "Invalid audio codec: %s\n", osmo_quote_str(audio_name, -1));
 		goto error;
+	}
 
 	/* Note: We only accept configurations with one audio channel! */
-	if (channels != 1)
+	if (channels != 1) {
+		LOGP(DLMGCP, LOGL_ERROR, "Cannot handle audio codec with more than one channel: %s\n",
+		     osmo_quote_str(audio_name, -1));
 		goto error;
+	}
 
 	codec->rate = rate;
 	codec->channels = channels;
@@ -186,6 +196,9 @@
 
 	/* Derive the payload type if it is unknown */
 	if (codec->payload_type == PTYPE_UNDEFINED) {
+		/* TODO: since all codecs are ultimately added by SDP lines like 'a=rtpmap:98 AMR/8000' I can't
+		 * currently see any practical use case for this condition: is there really ever a situation where the
+		 * payload type number is unknown in this function??? */
 
 		/* For the known codecs from the static range we restore
 		 * the IANA or 3GPP assigned payload type number */
@@ -221,9 +234,14 @@
 		 * 110 onwards 3gpp defines prefered codec types, which are
 		 * also fixed, see above)  */
 		if (codec->payload_type < 0) {
+			/* FIXME: pt_offset is completely unrelated and useless here, any of those numbers may already
+			 * have been added to the codecs. Instead, there should be an iterator checking for an actually
+			 * unused dynamic payload type number. */
 			codec->payload_type = 96 + pt_offset;
-			if (codec->payload_type > 109)
+			if (codec->payload_type > 109) {
+				LOGP(DLMGCP, LOGL_ERROR, "Ran out of payload type numbers to assign dynamically\n");
 				goto error;
+			}
 		}
 	}
 

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15138
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I0b44b574c814882b6f8ae7cd738a6f481cd721fd
Gerrit-Change-Number: 15138
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190809/771334b4/attachment.htm>