Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
neels has submitted this change and it was merged. (
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, 25 insertions(+), 9 deletions(-)
Approvals:
Jenkins Builder: Verified
osmith: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c
index 7c8f6d5..704b7e6 100644
--- a/src/libosmo-mgcp/mgcp_codec.c
+++ b/src/libosmo-mgcp/mgcp_codec.c
@@ -138,12 +138,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;
}
@@ -169,6 +170,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;
}
}
@@ -176,16 +179,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, , )
< 1)
+ if (sscanf(audio_name, "%63[^/]/%d/%d", audio_codec, , )
< 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;
@@ -203,6 +213,7 @@
/* Derive the payload type if it is unknown */
if (codec->payload_type == PTYPE_UNDEFINED) {
+ /* TODO: This is semi dead code, see OS#4150 */
/* For the known codecs from the static range we restore
* the IANA or 3GPP assigned payload type number */
@@ -238,9 +249,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
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15138 ) Change subject: mgcp_codec: codec_set(): log about all possible errors .. Patch Set 5: Code-Review+2 -- 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: 5 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 28 Aug 2019 09:09:08 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15138 ) Change subject: mgcp_codec: codec_set(): log about all possible errors .. Patch Set 5: Code-Review+1 -- 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: 5 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 28 Aug 2019 07:28:41 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/15138 )
Change subject: mgcp_codec: codec_set(): log about all possible errors
..
Patch Set 4:
(1 comment)
https://gerrit.osmocom.org/#/c/15138/4/src/libosmo-mgcp/mgcp_codec.c
File src/libosmo-mgcp/mgcp_codec.c:
https://gerrit.osmocom.org/#/c/15138/4/src/libosmo-mgcp/mgcp_codec.c@182
PS4, Line 182: if (strlen(audio_name) > sizeof(audio_codec)) {
> this should probably be >=
unrelated to this patch, which is just about logging. fixing separately.
--
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: 4
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 28 Aug 2019 02:55:42 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/15138 )
Change subject: mgcp_codec: codec_set(): log about all possible errors
..
Patch Set 4:
(1 comment)
https://gerrit.osmocom.org/#/c/15138/4/src/libosmo-mgcp/mgcp_codec.c
File src/libosmo-mgcp/mgcp_codec.c:
https://gerrit.osmocom.org/#/c/15138/4/src/libosmo-mgcp/mgcp_codec.c@182
PS4, Line 182: if (strlen(audio_name) > sizeof(audio_codec)) {
this should probably be >=
--
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: 4
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 14 Aug 2019 09:10:52 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
Hello pespin, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/15138 to look at the new patch set (#4). 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, 25 insertions(+), 9 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/38/15138/4 -- 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: 4 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15138 ) Change subject: mgcp_codec: codec_set(): log about all possible errors .. Patch Set 3: (1 comment) https://gerrit.osmocom.org/#/c/15138/3/src/libosmo-mgcp/mgcp_codec.c File src/libosmo-mgcp/mgcp_codec.c: https://gerrit.osmocom.org/#/c/15138/3/src/libosmo-mgcp/mgcp_codec.c@218 PS3, Line 218: * payload type number is unknown in this function??? */ > you may find information with git blame? Or send an email to ML asking the > author. […] This dynamic number (e.g. 96) would be the one stored in codec->payload_type. But I found it... it's for codecs in the MGCP header, without SDP. Opened https://osmocom.org/issues/4150 So that's for a different issue then. -- 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: 3 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 13 Aug 2019 23:03:55 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15138 ) Change subject: mgcp_codec: codec_set(): log about all possible errors .. Patch Set 3: (1 comment) https://gerrit.osmocom.org/#/c/15138/3/src/libosmo-mgcp/mgcp_codec.c File src/libosmo-mgcp/mgcp_codec.c: https://gerrit.osmocom.org/#/c/15138/3/src/libosmo-mgcp/mgcp_codec.c@218 PS3, Line 218: * payload type number is unknown in this function??? */ you may find information with git blame? Or send an email to ML asking the author. Without know much about this topic, reading comments below makes me thing it's in the case where dynamic payload type numbers are used to indicate well-known codecs which can be represented without dynamic payload numbers. BUt what I'm saying could be nonsense :) -- 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: 3 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 13 Aug 2019 09:42:10 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15138 ) Change subject: mgcp_codec: codec_set(): log about all possible errors .. Patch Set 1: Code-Review+1 -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-Comment-Date: Fri, 09 Aug 2019 11:16:46 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgcp_codec: codec_set(): log about all possible errors
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, , )
< 1)
+ if (sscanf(audio_name, "%63[^/]/%d/%d", audio_codec, , )
< 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,
