Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
pespin has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/osmo-mgw/+/14589 )
Change subject: mgw: Support uppercase LCO options
..
mgw: Support uppercase LCO options
MGCP RFC3435 (https://tools.ietf.org/html/rfc3435) states almost all
text has to be handled in a case-insensitive way, except SDP parts.
Related: OS#4001
Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d
---
M src/libosmo-mgcp/mgcp_protocol.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
3 files changed, 77 insertions(+), 21 deletions(-)
Approvals:
Jenkins Builder: Verified
pespin: Looks good to me, approved
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index 74926ad..9baf50d 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -590,7 +590,7 @@
static int set_local_cx_options(void *ctx, struct mgcp_lco *lco,
const char *options)
{
- char *p_opt, *a_opt;
+ char *lco_id;
char codec[17];
if (!options)
@@ -608,18 +608,33 @@
talloc_free(lco->string);
lco->string = talloc_strdup(ctx, options);
- p_opt = strstr(lco->string, "p:");
- if (p_opt && sscanf(p_opt, "p:%d-%d",
- >pkt_period_min, >pkt_period_max) == 1)
- lco->pkt_period_max = lco->pkt_period_min;
+ lco_id = lco->string;
+ while ((lco_id = get_lco_identifier(lco_id))) {
+ switch (tolower(lco_id[0])) {
+ case 'p':
+ if (sscanf(lco_id + 1, ":%d-%d",
+ >pkt_period_min, >pkt_period_max)
== 1)
+ lco->pkt_period_max = lco->pkt_period_min;
+ break;
+ case 'a':
+ /* FIXME: LCO also supports the negotiation of more
then one codec.
+* (e.g. a:PCMU;G726-32) But this implementation only
supports a single
+* codec only. */
+ if (sscanf(lco_id + 1, ":%16[^,]", codec) == 1) {
+ talloc_free(lco->codec);
+ lco->codec = talloc_strdup(ctx, codec);
+ }
+ break;
+ default:
+ LOGP(DLMGCP, LOGL_NOTICE,
+"LCO: unhandled option: '%c'/%d in \"%s\"\n",
+*lco_id, *lco_id, lco->string);
+ break;
+ }
- /* FIXME: LCO also supports the negotiation of more then one codec.
-* (e.g. a:PCMU;G726-32) But this implementation only supports a single
-* codec only. */
- a_opt = strstr(lco->string, "a:");
- if (a_opt && sscanf(a_opt, "a:%16[^,]", codec) == 1) {
- talloc_free(lco->codec);
- lco->codec = talloc_strdup(ctx, codec);
+ lco_id = strchr(lco_id, ',');
+ if (!lco_id)
+ break;
}
LOGP(DLMGCP, LOGL_DEBUG,
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index c4931b2..ab6d0ce 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -208,8 +208,24 @@
"a=rtpmap:99 AMR/8000\r\n" \
"a=ptime:40\r\n"
-#define MDCX4_SO \
+/* Test different upper/lower case in options */
+#define MDCX4_PT4 \
"MDCX 18983220 1@mgw MGCP 1.0\r\n" \
+ "M: sendrecv\r" \
+ "C: 2\r\n" \
+ "I: %s\r\n" \
+ "L: A:AMR, NT:IN\r\n" \
+ "\n" \
+ "v=0\r\n" \
+ "o=- %s 23 IN IP4 0.0.0.0\r\n" \
+ "c=IN IP4 0.0.0.0\r\n" \
+ "t=0 0\r\n" \
+ "m=audio 4441 RTP/AVP 99\r\n" \
+ "a=rtpmap:99 AMR/8000\r\n" \
+ "a=ptime:40\r\n"
+
+#define MDCX4_SO \
+ "MDCX 18983221 1@mgw MGCP 1.0\r\n" \
"M: sendonly\r" \
"C: 2\r\n" \
"I: %s\r\n" \
@@ -224,17 +240,17 @@
"a=ptime:40\r\n"
#define MDCX4_RO \
- "MDCX 18983221 1@mgw MGCP 1.0\r\n" \
+ "MDCX 18983222 1@mgw MGCP 1.0\r\n" \
"M: recvonly\r" \
"C: 2\r\n" \
"I: %s\r\n" \
"L: p:20, a:AMR, nt:IN\r\n"
#define MDCX_TOO_LONG_CI \
- "MDCX 18983222 1@mgw MGCP 1.0\r\n" \
+ "MDCX 18983223 1@mgw MGCP 1.0\r\n" \
"I: 123456789012345678901234567890123\n"
-#define MDCX_TOO_LONG_CI_RET "510 18983222 FAIL\r\n"
+#define MDCX_TOO_LONG_CI_RET "510 18983223 FAIL\r\n"
#define SHORT2 "CRCX 1"
#define SHORT2_RET "510 00 FAIL\r\n"
@@ -526,8 +542,9 @@
{"MDCX4_PT1", MDCX4_PT1, MDCX4_RET("18983217"), 99},
{"MDCX4_PT2", MDCX4_PT2, MDCX4_RET("18983218"), 99},
{"MDCX4_PT3", MDCX4_PT3, MDCX4_RET("18983219"), 99},
- {"MDCX4_SO", MDCX4_SO, MDCX4_RET("18983220"), 99},
- {"MDCX4_RO", MDCX4_RO, MDCX4_RO_RET("18983221"), PTYPE_IGNORE},
+ {"MDCX4_PT4", MDCX4_PT4, MDCX4_RET("18983220"), 99},
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/14589 ) Change subject: mgw: Support uppercase LCO options .. Patch Set 2: Code-Review+2 Adding +2 after applying osmith's comment. -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d Gerrit-Change-Number: 14589 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 03 Jul 2019 10:39:44 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/14589 )
Change subject: mgw: Support uppercase LCO options
..
Patch Set 2:
(1 comment)
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c@612
PS1, Line 612: while ((lco_id = get_lco_identifier(lco_id))) {
> After reading https://gerrit.osmocom. […]
Yes, check_local_cx_options() takes care of it so that scenario cannot happen.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d
Gerrit-Change-Number: 14589
Gerrit-PatchSet: 2
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 03 Jul 2019 10:37:02 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith
Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
Hello daniel, laforge, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589 to look at the new patch set (#2). Change subject: mgw: Support uppercase LCO options .. mgw: Support uppercase LCO options MGCP RFC3435 (https://tools.ietf.org/html/rfc3435) states almost all text has to be handled in a case-insensitive way, except SDP parts. Related: OS#4001 Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d --- M src/libosmo-mgcp/mgcp_protocol.c M tests/mgcp/mgcp_test.c M tests/mgcp/mgcp_test.ok 3 files changed, 77 insertions(+), 21 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/89/14589/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d Gerrit-Change-Number: 14589 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
osmith has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/14589 )
Change subject: mgw: Support uppercase LCO options
..
Patch Set 1:
(1 comment)
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c@612
PS1, Line 612: while ((lco_id = get_lco_identifier(lco_id))) {
> This is changing the parsing logic slightly. […]
After reading https://gerrit.osmocom.org/c/osmo-mgw/+/14597/1 I realize that
check_local_cx_options() takes care of the case above.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d
Gerrit-Change-Number: 14589
Gerrit-PatchSet: 1
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 03 Jul 2019 07:34:08 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith
Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
osmith has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-mgw/+/14589 )
Change subject: mgw: Support uppercase LCO options
..
Patch Set 1: Code-Review+1
(2 comments)
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c@612
PS1, Line 612: while ((lco_id = get_lco_identifier(lco_id))) {
This is changing the parsing logic slightly. It is probably worth mentioning in
the commit message, just in case we have a regression.
Here's an example where the result would not be the same (probably not a real
world use case; old code uses first p:, new code uses second p:):
p:1234-1234, a:PCUM, p:2345-2345
https://gerrit.osmocom.org/#/c/14589/1/src/libosmo-mgcp/mgcp_protocol.c@630
PS1, Line 630: "LCO: unhandled option: '%c'/%d\n",
*lco_id, *lco_id);
How about also logging the entire lco->string here?
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d
Gerrit-Change-Number: 14589
Gerrit-PatchSet: 1
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 03 Jul 2019 07:18:11 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/14589 ) Change subject: mgw: Support uppercase LCO options .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d Gerrit-Change-Number: 14589 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Comment-Date: Thu, 27 Jun 2019 01:22:27 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/14589 ) Change subject: mgw: Support uppercase LCO options .. Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/14589 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d Gerrit-Change-Number: 14589 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Comment-Date: Thu, 27 Jun 2019 01:21:49 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: mgw: Support uppercase LCO options
pespin has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-mgw/+/14589
Change subject: mgw: Support uppercase LCO options
..
mgw: Support uppercase LCO options
MGCP RFC3435 (https://tools.ietf.org/html/rfc3435) states almost all
text has to be handled in a case-insensitive way, except SDP parts.
Related: OS#4001
Change-Id: Ic28a5eacc4c441d68e8a20d2743956ab2e01125d
---
M src/libosmo-mgcp/mgcp_protocol.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
3 files changed, 76 insertions(+), 21 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/89/14589/1
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index 74926ad..841440b 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -590,7 +590,7 @@
static int set_local_cx_options(void *ctx, struct mgcp_lco *lco,
const char *options)
{
- char *p_opt, *a_opt;
+ char *lco_id;
char codec[17];
if (!options)
@@ -608,18 +608,32 @@
talloc_free(lco->string);
lco->string = talloc_strdup(ctx, options);
- p_opt = strstr(lco->string, "p:");
- if (p_opt && sscanf(p_opt, "p:%d-%d",
- >pkt_period_min, >pkt_period_max) == 1)
- lco->pkt_period_max = lco->pkt_period_min;
+ lco_id = lco->string;
+ while ((lco_id = get_lco_identifier(lco_id))) {
+ switch (tolower(lco_id[0])) {
+ case 'p':
+ if (sscanf(lco_id + 1, ":%d-%d",
+ >pkt_period_min, >pkt_period_max)
== 1)
+ lco->pkt_period_max = lco->pkt_period_min;
+ break;
+ case 'a':
+ /* FIXME: LCO also supports the negotiation of more
then one codec.
+* (e.g. a:PCMU;G726-32) But this implementation only
supports a single
+* codec only. */
+ if (sscanf(lco_id + 1, ":%16[^,]", codec) == 1) {
+ talloc_free(lco->codec);
+ lco->codec = talloc_strdup(ctx, codec);
+ }
+ break;
+ default:
+ LOGP(DLMGCP, LOGL_NOTICE,
+"LCO: unhandled option: '%c'/%d\n", *lco_id,
*lco_id);
+ break;
+ }
- /* FIXME: LCO also supports the negotiation of more then one codec.
-* (e.g. a:PCMU;G726-32) But this implementation only supports a single
-* codec only. */
- a_opt = strstr(lco->string, "a:");
- if (a_opt && sscanf(a_opt, "a:%16[^,]", codec) == 1) {
- talloc_free(lco->codec);
- lco->codec = talloc_strdup(ctx, codec);
+ lco_id = strchr(lco_id, ',');
+ if (!lco_id)
+ break;
}
LOGP(DLMGCP, LOGL_DEBUG,
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index c4931b2..ab6d0ce 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -208,8 +208,24 @@
"a=rtpmap:99 AMR/8000\r\n" \
"a=ptime:40\r\n"
-#define MDCX4_SO \
+/* Test different upper/lower case in options */
+#define MDCX4_PT4 \
"MDCX 18983220 1@mgw MGCP 1.0\r\n" \
+ "M: sendrecv\r" \
+ "C: 2\r\n" \
+ "I: %s\r\n" \
+ "L: A:AMR, NT:IN\r\n" \
+ "\n" \
+ "v=0\r\n" \
+ "o=- %s 23 IN IP4 0.0.0.0\r\n" \
+ "c=IN IP4 0.0.0.0\r\n" \
+ "t=0 0\r\n" \
+ "m=audio 4441 RTP/AVP 99\r\n" \
+ "a=rtpmap:99 AMR/8000\r\n" \
+ "a=ptime:40\r\n"
+
+#define MDCX4_SO \
+ "MDCX 18983221 1@mgw MGCP 1.0\r\n" \
"M: sendonly\r" \
"C: 2\r\n" \
"I: %s\r\n" \
@@ -224,17 +240,17 @@
"a=ptime:40\r\n"
#define MDCX4_RO \
- "MDCX 18983221 1@mgw MGCP 1.0\r\n" \
+ "MDCX 18983222 1@mgw MGCP 1.0\r\n" \
"M: recvonly\r" \
"C: 2\r\n" \
"I: %s\r\n" \
"L: p:20, a:AMR, nt:IN\r\n"
#define MDCX_TOO_LONG_CI \
- "MDCX 18983222 1@mgw MGCP 1.0\r\n" \
+ "MDCX 18983223 1@mgw MGCP 1.0\r\n" \
"I: 123456789012345678901234567890123\n"
-#define MDCX_TOO_LONG_CI_RET "510 18983222 FAIL\r\n"
+#define MDCX_TOO_LONG_CI_RET "510 18983223 FAIL\r\n"
#define SHORT2 "CRCX 1"
#define SHORT2_RET "510 00 FAIL\r\n"
@@ -526,8 +542,9 @@
{"MDCX4_PT1", MDCX4_PT1, MDCX4_RET("18983217"), 99},
{"MDCX4_PT2", MDCX4_PT2, MDCX4_RET("18983218"), 99},
{"MDCX4_PT3", MDCX4_PT3, MDCX4_RET("18983219"), 99},
- {"MDCX4_SO", MDCX4_SO, MDCX4_RET("18983220"), 99},
- {"MDCX4_RO", MDCX4_RO, MDCX4_RO_RET("18983221"), PTYPE_IGNORE},
+ {"MDCX4_PT4", MDCX4_PT4, MDCX4_RET("18983220"), 99},
+ {"MDCX4_SO", MDCX4_SO, MDCX4_RET("18983221"), 99},
+
