Attention is currently required from: neels.
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ci/+/32230 )
Change subject: jobs/osmocom-obs-rhizomatica: add new jobs
......................................................................
Patch Set 2:
(1 comment)
File jobs/osmocom-obs-rhizomatica.yml:
https://gerrit.osmocom.org/c/osmo-ci/+/32230/comment/0b69ac7f_45ce51c9
PS2, Line 48: --feed "master" \
> does it show in the naming/the URL
no, that is set by PROJ.
> is it freely choosable
It must be master for this type of repository. The --feed argument used to describe the feed we are uploading to (nightly, latest, master). It tells the update_obs_project.py script:
* which git reference it should try to upload (latest: the last release tag, master/nightly: the last commit on the given git branch, by default the "master" git branch)
* whether it should store the current git as text file in OBS and compare it ahead of time, and skip the package if it didn't change (master only)
Now that we have additional repositories (wireshark, rhizomatica), it isn't necessarily the actual feed we are uploading to. It only describes the characteristics named above. So the name is historic here, it probably makes sense to give it another name to avoid confusion. I'll do that later in a follow-up patch.
--
To view, visit https://gerrit.osmocom.org/c/osmo-ci/+/32230
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ci
Gerrit-Branch: master
Gerrit-Change-Id: I9d59f517eb4ecfd721ef1dba4519a874590f756e
Gerrit-Change-Number: 32230
Gerrit-PatchSet: 2
Gerrit-Owner: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 14:05:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/32218
to look at the new patch set (#5).
Change subject: mgcp_codec: fix codec decision
......................................................................
mgcp_codec: fix codec decision
Unfortunately OsmoMGW was never really tested with multiple different
codecs on either side of the connection. While OsmoMSC and OsmoBSC only
assign exactly one codec on each side this has never been a problem,
however it might become a problem when a call againt assigns multiple
codecs on one side. This has been observed in a setup where OsmoMGW had
one leg towards an external call agent. Also due to recent upgrades to
the TTCN3 tests we are now able to simulate different codecs on both
sides to pinpoint issues.
Testing has shown that OsmoMGW has difficulties with multiple codecs.
The reason for this is that the function that makes the codec decision
was not fully finished. So let's finish the codec decision function and
let's also use that decision when patching the payload type of outgoing
RTP packets.
Related: OS#5461
Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
---
M include/osmocom/mgcp/mgcp_codec.h
M include/osmocom/mgcp/mgcp_trunk.h
M src/libosmo-mgcp/mgcp_codec.c
M src/libosmo-mgcp/mgcp_network.c
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_vty.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
8 files changed, 289 insertions(+), 261 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/18/32218/5
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 )
Change subject: mgcp_codec: fix codec decision
......................................................................
Patch Set 5:
(3 comments)
File src/libosmo-mgcp/mgcp_codec.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/932b783e_30d440a2
PS2, Line 440: conn->end.codec = mgcp_codec_find_same(conn, found_same_codec_opposite);
> Ah I see what you mean, you are looking the codec in each conn list. […]
The data is not duplicated. Its just that each endpoint holds two connections and each connections has a codec list. This is the list that had been issued by the call agent during CRCX/MDCX. We can not put both into one list since then we would lose the info which side supports what.
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/f8e569a8_0e2f5fe9
PS4, Line 509: return;
> so you decided to drop the return code while still logging an ERROR. […]
The caller is not doing anything meaningful with it but maybe it is better to drop the packet than sending it with a wrong PT.
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/5f1a8dad_f78b9f05
PS4, Line 1166: mgcp_patch_pt(conn_dst, msg);
> all this code path failing looks buggy. […]
Technically nothing has changed but I can see mgcp_send() might need some refactoring.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 14:05:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: osmith.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ci/+/32230 )
Change subject: jobs/osmocom-obs-rhizomatica: add new jobs
......................................................................
Patch Set 2:
(1 comment)
File jobs/osmocom-obs-rhizomatica.yml:
https://gerrit.osmocom.org/c/osmo-ci/+/32230/comment/9eb4abeb_0a2b4b08
PS2, Line 48: --feed "master" \
I'm wondering about this "master" name: does it show in the naming/the URL, and is it freely choosable? because this is not the origin/master feed, but based on a custom rhizomatica branch, right? Could this be confusing, vs origin/master?
--
To view, visit https://gerrit.osmocom.org/c/osmo-ci/+/32230
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ci
Gerrit-Branch: master
Gerrit-Change-Id: I9d59f517eb4ecfd721ef1dba4519a874590f756e
Gerrit-Change-Number: 32230
Gerrit-PatchSet: 2
Gerrit-Owner: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 13:54:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 )
Change subject: mgcp_codec: fix codec decision
......................................................................
Patch Set 4:
(3 comments)
File src/libosmo-mgcp/mgcp_network.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/f8984686_c8f541bf
PS4, Line 509: return;
so you decided to drop the return code while still logging an ERROR. WHY?
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/59799fe7_89329e4d
PS4, Line 1166: mgcp_patch_pt(conn_dst, msg);
all this code path failing looks buggy. Looks like it was buggy before, but now it looks even worse?
File src/libosmo-mgcp/mgcp_vty.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/0471b13a_8cc51dee
PS4, Line 703: "allow-transcoding", "Allow transcoding\n")
These are still missing the deprecated warnings?
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 4
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 12:20:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 )
Change subject: mgcp_codec: fix codec decision
......................................................................
Patch Set 4:
(1 comment)
File src/libosmo-mgcp/mgcp_codec.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/eae3fd85_ed27682e
PS2, Line 440: conn->end.codec = mgcp_codec_find_same(conn, found_same_codec_opposite);
> The problem is that it is not possible to assign codec_conn_dst to conn_src->end. […]
Ah I see what you mean, you are looking the codec in each conn list.
Now the question I have is: why do we have codec structs holding the same data duplicated (allocated several times) for each connection?
Wouldn't it make more sense to have them defined in 1 global place and then have a pointer to that global struct here in all conns?
I'm not saying it has to be done in this commit, just pointing out some possibilities for improvement.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 4
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 06 Apr 2023 12:17:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/32218
to look at the new patch set (#4).
Change subject: mgcp_codec: fix codec decision
......................................................................
mgcp_codec: fix codec decision
Unfortunately OsmoMGW was never really tested with multiple different
codecs on either side of the connection. While OsmoMSC and OsmoBSC only
assign exactly one codec on each side this has never been a problem,
however it might become a problem when a call againt assigns multiple
codecs on one side. This has been observed in a setup where OsmoMGW had
one leg towards an external call agent. Also due to recent upgrades to
the TTCN3 tests we are now able to simulate different codecs on both
sides to pinpoint issues.
Testing has shown that OsmoMGW has difficulties with multiple codecs.
The reason for this is that the function that makes the codec decision
was not fully finished. So let's finish the codec decision function and
let's also use that decision when patching the payload type of outgoing
RTP packets.
Related: OS#5461
Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
---
M include/osmocom/mgcp/mgcp_codec.h
M include/osmocom/mgcp/mgcp_trunk.h
M src/libosmo-mgcp/mgcp_codec.c
M src/libosmo-mgcp/mgcp_network.c
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_vty.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
8 files changed, 287 insertions(+), 266 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/18/32218/4
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5
Gerrit-Change-Number: 32218
Gerrit-PatchSet: 4
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset