Attention is currently required from: fixeria, daniel, lynxis lazus.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32176 )
Change subject: gprs: fix has_valid_nsvc(): permit local udp port 0
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
(Copy-pasting from previous Change-Id:)
The question here is more: does it make sense from network architecture point of view that PCU binds to a randomly available port for Gb?
AFAIU it makes no sense, since it needs to bind to a specific port also known by the SGSN so that it knows where to send packets to. Or is the SGSN available to learn dynamically of the PCU by listening() on its port?
Maybe something @lynxis or @daniel will be able to answer better.
If it makes no sense that the PCU can bidn to a random port, then sure, let's disallow passing bind port 0. But let's then explain correctly the rationale behind it, which is wrong in current version of this patch.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32176
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I0afd83e77f3daeeb082e7db911610428b5bc587c
Gerrit-Change-Number: 32176
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Fri, 31 Mar 2023 14:28:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32171 )
Change subject: tests: add more tests for GPRS NSVC parameters
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> I am not really liking this approach of "port 0" meaning the NSVC is disabled. […]
Neither I do. But we have what we have. I am not changing the logic.
This patch is not introducing this approach, but demonstrating it.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32171
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I13d6ac887ddb1c9bc5d1e4122f20405a28f135d4
Gerrit-Change-Number: 32171
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 31 Mar 2023 14:27:41 +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: laforge, pespin, daniel, lynxis lazus.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32169 )
Change subject: tests: rename and extend gprs_{bvci_default->params}.vty
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Patchset:
PS2:
Ok, this is a totally new commit after latest version submit, and the discussion doesn't apply anymore?
Not sure if it was the best idea to reuse the Change-Id here.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32169
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ia02e9a97137eb7724f22526fc3768331b35ac55d
Gerrit-Change-Number: 32169
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Fri, 31 Mar 2023 14:26:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: laforge, pespin, daniel, lynxis lazus.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32169 )
Change subject: tests: rename and extend gprs_{bvci_default->params}.vty
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-bsc/+/32169/comment/cb9ed249_3822af49
PS1, Line 18: The PCU obviously cannot bind to port 0, and will keep re-starting.
> The question here is more: does it make sense from network architecture point of view that PCU binds […]
I investigated this a bit more and found out that 0 is a perfectly valid value, which has been supported until the following patch was merged:
```
commit 315af2f9ea1e8b9bf6e58caebd9dd7829edecfed
Author: Alexander Couzens <lynxis(a)fe80.eu>
Date: Mon Dec 19 21:21:32 2022 +0100
```
See https://gerrit.osmocom.org/c/osmo-bsc/+/32176. I confirm that with this patch applied osmo-pcu happily connects to osmo-sgsn and GPRS service works as expected.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32169
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ia02e9a97137eb7724f22526fc3768331b35ac55d
Gerrit-Change-Number: 32169
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Fri, 31 Mar 2023 14:26:00 +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>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32171 )
Change subject: tests: add more tests for GPRS NSVC parameters
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
I am not really liking this approach of "port 0" meaning the NSVC is disabled. We should have some sort of explicitly enabling and disabling NSVCs.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32171
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I13d6ac887ddb1c9bc5d1e4122f20405a28f135d4
Gerrit-Change-Number: 32171
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 31 Mar 2023 14:25:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32176 )
Change subject: gprs: fix has_valid_nsvc(): permit local udp port 0
......................................................................
gprs: fix has_valid_nsvc(): permit local udp port 0
NSVC local port 0 is actually a valid value, which tells the PCU to
pick a random port for bind()ing. It was supported before 315af2f9e,
but now osmo-bsc would simply ignore NSVCs with 'local udp port 0'
and leave the respective MOs unconfigured in the BTS.
Change-Id: I0afd83e77f3daeeb082e7db911610428b5bc587c
Fixes: 315af2f9e "bts: ipa/osmo-bts/sysmobts: MO: add support for the second NSVC"
Related: OS#5979
---
M src/osmo-bsc/nm_gprs_nsvc_fsm.c
1 file changed, 22 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/76/32176/1
diff --git a/src/osmo-bsc/nm_gprs_nsvc_fsm.c b/src/osmo-bsc/nm_gprs_nsvc_fsm.c
index 5186082..9eb6216 100644
--- a/src/osmo-bsc/nm_gprs_nsvc_fsm.c
+++ b/src/osmo-bsc/nm_gprs_nsvc_fsm.c
@@ -92,10 +92,15 @@
static bool has_valid_nsvc(struct gsm_gprs_nsvc *nsvc)
{
+ /* remote address must be valid */
+ if (osmo_sockaddr_is_any(&nsvc->remote))
+ return false;
+ /* remote port must be valid */
switch (nsvc->remote.u.sa.sa_family) {
case AF_INET:
+ return nsvc->remote.u.sin.sin_port != 0;
case AF_INET6:
- return (nsvc->local_port > 0 && !osmo_sockaddr_is_any(&nsvc->remote));
+ return nsvc->remote.u.sin6.sin6_port != 0;
default:
return false;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32176
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I0afd83e77f3daeeb082e7db911610428b5bc587c
Gerrit-Change-Number: 32176
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: laforge, fixeria, daniel, lynxis lazus.
Hello Jenkins Builder, laforge, daniel, lynxis lazus,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/32169
to look at the new patch set (#2).
Change subject: tests: rename and extend gprs_{bvci_default->params}.vty
......................................................................
tests: rename and extend gprs_{bvci_default->params}.vty
Match default values for all GPRS related params, not only the BVCI.
Change-Id: Ia02e9a97137eb7724f22526fc3768331b35ac55d
Related: OS#5979
---
D tests/gprs_bvci_default.vty
A tests/gprs_params.vty
2 files changed, 60 insertions(+), 13 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/69/32169/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32169
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ia02e9a97137eb7724f22526fc3768331b35ac55d
Gerrit-Change-Number: 32169
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-MessageType: newpatchset
Attention is currently required from: fixeria.
Hello Jenkins Builder, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/32171
to look at the new patch set (#2).
Change subject: tests: add more tests for GPRS NSVC parameters
......................................................................
tests: add more tests for GPRS NSVC parameters
Change-Id: I13d6ac887ddb1c9bc5d1e4122f20405a28f135d4
Related: OS#5979
---
M tests/gprs_params.vty
1 file changed, 78 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/71/32171/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32171
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I13d6ac887ddb1c9bc5d1e4122f20405a28f135d4
Gerrit-Change-Number: 32171
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin, fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/32070 )
Change subject: abis_nm: Only osmo-bts re-purposes the MANUF_ID for BTS feature flags
......................................................................
Patch Set 3:
(1 comment)
File src/osmo-bsc/abis_nm.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/32070/comment/44fd65b4_576cd8f5
PS2, Line 576: if (bts->type == GSM_BTS_TYPE_OSMOBTS && TLVP_PRES_LEN(tp, NM_ATT_MANUF_ID, 2)) {
> Probably a switch (bts->type) may be cleaner here.
I decided to do away with any nanoBTS handling at this point, as it simply doesn't apply. The features are static. So we don't need a switch as it's a single if-clause.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/32070
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I76cee190dc1f074464df570cdfc3d38559f04846
Gerrit-Change-Number: 32070
Gerrit-PatchSet: 3
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 31 Mar 2023 14:13:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment