neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31712 )
Change subject: fix coverity (false) warning in codec-list vty
......................................................................
fix coverity (false) warning in codec-list vty
Since there were complaints about this old parsing code during recent
code review in Ifdc9e04bf1d623da65bfb8a2fddea765601f6d9b, and now also
coverity finds something odd in it, just rewrite this.
Related: CID#310912
Change-Id: I96cd5d88ec6808a2915c6bccd0c0140216f328f2
---
M src/osmo-bsc/bsc_vty.c
M tests/msc.vty
2 files changed, 41 insertions(+), 29 deletions(-)
Approvals:
fixeria: Looks good to me, approved
pespin: Looks good to me, but someone else must approve
Jenkins Builder: Verified
diff --git a/src/osmo-bsc/bsc_vty.c b/src/osmo-bsc/bsc_vty.c
index e14aecb..467301b 100644
--- a/src/osmo-bsc/bsc_vty.c
+++ b/src/osmo-bsc/bsc_vty.c
@@ -2712,6 +2712,7 @@
{
struct bsc_msc_data *data = bsc_msc_data(vty);
struct gsm_audio_support tmp[ARRAY_SIZE(data->audio_support)];
+ const char *arg = NULL;
int i;
/* Nr of arguments must fit in the array */
@@ -2724,30 +2725,23 @@
/* check all given arguments first */
for (i = 0; i < argc; i++) {
int j;
+ int ver;
+ arg = argv[i];
- /* check for hrX or frX */
- if (strlen(argv[i]) != 3
- || argv[i][1] != 'r'
- || (argv[i][0] != 'h' && argv[i][0] != 'f')
- || argv[i][2] < '0'
- || argv[i][2] > '9') {
- vty_out(vty, "Codec name must be hrX or frX. Was '%s'%s", argv[i], VTY_NEWLINE);
- return CMD_WARNING;
- }
-
- /* store in tmp[] first, to not overwrite data->audio_support[] in case of error */
- tmp[i].ver = atoi(argv[i] + 2);
- if (strncmp("hr", argv[i], 2) == 0)
+ if (strncmp("hr", arg, 2) == 0)
tmp[i].hr = 1;
- else if (strncmp("fr", argv[i], 2) == 0)
+ else if (strncmp("fr", arg, 2) == 0)
tmp[i].hr = 0;
+ else
+ goto invalid_arg;
- /* forbid invalid versions */
- if (tmp[i].ver < 1 || tmp[i].ver > 7
- || (tmp[i].hr && tmp[i].ver == 2)) {
- vty_out(vty, "'%s' is not a valid codec version%s", argv[i], VTY_NEWLINE);
- return CMD_WARNING;
- }
+ ver = atoi(arg + 2);
+ if (ver < 1 || ver > 7)
+ goto invalid_arg;
+ tmp[i].ver = ver;
+
+ if (tmp[i].hr == 1 && ver == 2)
+ goto invalid_arg;
/* prevent duplicate entries */
for (j = 0; j < i; j++) {
@@ -2762,6 +2756,10 @@
data->audio_length = argc;
return CMD_SUCCESS;
+
+invalid_arg:
+ vty_out(vty, "%s is not a valid codec version%s", osmo_quote_cstr_c(OTC_SELECT, arg, -1), VTY_NEWLINE);
+ return CMD_WARNING;
}
#define LEGACY_STR "This command has no effect, it is kept to support legacy config files\n"
diff --git a/tests/msc.vty b/tests/msc.vty
index d67f2b9..08c7f71 100644
--- a/tests/msc.vty
+++ b/tests/msc.vty
@@ -33,16 +33,16 @@
...
OsmoBSC(config-msc)# codec-list foo
-Codec name must be hrX or frX. Was 'foo'
+"foo" is not a valid codec version
OsmoBSC(config-msc)# codec-list fr10
-Codec name must be hrX or frX. Was 'fr10'
+"fr10" is not a valid codec version
OsmoBSC(config-msc)# codec-list hr10
-Codec name must be hrX or frX. Was 'hr10'
+"hr10" is not a valid codec version
OsmoBSC(config-msc)# codec-list FR1
-Codec name must be hrX or frX. Was 'FR1'
+"FR1" is not a valid codec version
OsmoBSC(config-msc)# # Ensure the codec-list with wrong args did not change the config
OsmoBSC(config-msc)# show running-config
@@ -62,7 +62,7 @@
...
OsmoBSC(config-msc)# codec-list fr0 fr1
-'fr0' is not a valid codec version
+"fr0" is not a valid codec version
OsmoBSC(config-msc)# show running-config
...
msc 0
@@ -71,7 +71,7 @@
...
OsmoBSC(config-msc)# codec-list hr0 hr1
-'hr0' is not a valid codec version
+"hr0" is not a valid codec version
OsmoBSC(config-msc)# show running-config
...
msc 0
@@ -80,7 +80,7 @@
...
OsmoBSC(config-msc)# codec-list fr8 fr9
-'fr8' is not a valid codec version
+"fr8" is not a valid codec version
OsmoBSC(config-msc)# show running-config
...
msc 0
@@ -89,7 +89,7 @@
...
OsmoBSC(config-msc)# codec-list hr8 hr9
-'hr8' is not a valid codec version
+"hr8" is not a valid codec version
OsmoBSC(config-msc)# show running-config
...
msc 0
@@ -98,7 +98,7 @@
...
OsmoBSC(config-msc)# codec-list fr2 hr2
-'hr2' is not a valid codec version
+"hr2" is not a valid codec version
OsmoBSC(config-msc)# show running-config
...
msc 0
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31712
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I96cd5d88ec6808a2915c6bccd0c0140216f328f2
Gerrit-Change-Number: 31712
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/31740 )
Change subject: do CN CRCX first
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-msc/+/31740/comment/527f746a_9724b493
PS4, Line 9: In order to send the MSC's RTP endpoint IP address+port in the inital
> typo: initial
Ack
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/31740
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Ie433db1ba0c46d4b97538a969233c155cefac21c
Gerrit-Change-Number: 31740
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 18 Mar 2023 02:06:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels.
Hello Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-msc/+/31740
to look at the new patch set (#5).
Change subject: do CN CRCX first
......................................................................
do CN CRCX first
In order to send the MSC's RTP endpoint IP address+port in the initial
SDP, move the MGCP CRCX up to an earlier point in the sequence of
establishing a voice call.
Update the voice call sequence chart to show the effects.
Though the semantic change is rather simple, the patch is rather huge --
things have to happen in a different order, and async waits have to
happen at different times.
The new codec filter helps to carry codec resolution information across
the newly arranged code paths.
Related: SYS#5066
Change-Id: Ie433db1ba0c46d4b97538a969233c155cefac21c
---
M doc/sequence_charts/voice_call_external_mncc.msc
M doc/sequence_charts/voice_call_internal_mncc.msc
M include/osmocom/msc/gsm_04_08.h
M include/osmocom/msc/msc_a.h
M src/libmsc/gsm_04_08_cc.c
M src/libmsc/msc_a.c
M tests/msc_vlr/msc_vlr_test_call.c
M tests/msc_vlr/msc_vlr_test_call.err
8 files changed, 487 insertions(+), 261 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/40/31740/5
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/31740
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Ie433db1ba0c46d4b97538a969233c155cefac21c
Gerrit-Change-Number: 31740
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: laforge, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/31698 )
Change subject: [codecs filter] send + receive SDP via MNCC
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
> Ok, if others are fine with it let's merge it, though I really think this paradigm of having to call […]
codec_filter_run() does not log.
I believe the strong opinion is a gut feel that the inputs do not change often, that "filter_run()" sounds expensive, and that the results are not used often. All of these are not accurate impressions. Inputs update more often than results are being used, and codec_filter_run() is not expensive.
The amount of us discussing when to call run() far exceeds the significance of this aspect. You've asked about this over an extended period of time and in many different patches. I'm trying to convince you that it is not an important issue, but it seems my responses don't reach home.
Important at this point is that it has correct results. There are a number of quite non-trivial aspects of this patch series, IMHO this part is just a bike shed slash premature optimization, rather don't let review get distracted by this marginal aspect.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/31698
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Ie0668c0e079ec69da1532b52d00621efe114fc2c
Gerrit-Change-Number: 31698
Gerrit-PatchSet: 7
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 18 Mar 2023 02:03:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/30126 )
Change subject: [codecs filter] use filter result in Assignment
......................................................................
Patch Set 8:
(1 comment)
File src/libmsc/gsm_04_08_cc.c:
https://gerrit.osmocom.org/c/osmo-msc/+/30126/comment/1583eb47_86d845eb
PS4, Line 1797: codec_filter_run(&trans->cc.codecs);
> I think that was an unconclusive discussion, and probably I was not really happy about that 😊 […]
no, it's not expensive to call codec_filter_run().
In reverse I could ask: what's the point in calling it every time the inputs modify when nothing is using the result between input modifications?
Inputs are from various sources, e.g. BSSAP Codec List (BSS Supported), MS Bearer Capabilites, the SDP from the remote call leg. It is a simple way to just call codec_filter_run() before using its result.
Yes, we could also call it whenever any of the inputs change. We could also call it implicitly from every codec_filter_set_foo() function. Neither would make much of a difference. I chose to run() before using the result as a simple way to not call run() overly often, and I think there's nothing wrong with this approach.
If you would like to optimize performance, here is my usual answer: run a profiler to measure and prove the places that are inefficient; i'm pretty sure the codec filter is not a significant contender.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/30126
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I66e7c8c5e401f4f3a7d3d42b9525b2c6e99691d9
Gerrit-Change-Number: 30126
Gerrit-PatchSet: 8
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 18 Mar 2023 01:48:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31940 )
Change subject: Fix Lb/A SCCP conn lookup after recent regression in optimization patch
......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS1:
> I think this needs further study in terms of the SCCP/sigtran rlated data structures. […]
Done
Patchset:
PS2:
> Let me try to fix this today, otherwise I'll merge a revert.
Done
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31940
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: If42d93adee71d646766929a09bc01ae92b734ef3
Gerrit-Change-Number: 31940
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 17 Mar 2023 18:08:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment