July 2023 - gerrit-log - lists.osmocom.org

[M] Change in docker-playground[master]: */osmo-stp.cfg: Explicitly define role & sctp-role
by pespin 05 Jul '23

05 Jul '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/docker-playground/+/33544 ) Change subject: */osmo-stp.cfg: Explicitly define role & sctp-role ...................................................................... */osmo-stp.cfg: Explicitly define role & sctp-role It was recently decided it's a good practice to always specify the role and sctp-role for all ASPs configured in the VTY, since it's an important configuration providing feedback on the network setup expectancies. Change-Id: Ia495bc3c5dd4421e5730c74b2f5dc4e4cdc1a673 --- M nplab-m3ua-test/osmo-stp.cfg M nplab-sua-test/osmo-stp.cfg M osmo-stp-latest/osmo-stp.cfg M osmo-stp-master/osmo-stp.cfg M ttcn3-bsc-test-vamos/osmo-stp.cfg M ttcn3-bsc-test/osmo-stp.cfg M ttcn3-hnbgw-test/osmo-stp.cfg M ttcn3-msc-test/osmo-stp.cfg M ttcn3-sgsn-test/osmo-stp.cfg M ttcn3-smlc-test/osmo-stp.cfg M ttcn3-stp-test/osmo-stp.cfg 11 files changed, 77 insertions(+), 3 deletions(-) Approvals: fixeria: Looks good to me, approved osmith: Looks good to me, but someone else must approve Jenkins Builder: Verified diff --git a/nplab-m3ua-test/osmo-stp.cfg b/nplab-m3ua-test/osmo-stp.cfg index 90ad0d4..f315d77 100644 --- a/nplab-m3ua-test/osmo-stp.cfg +++ b/nplab-m3ua-test/osmo-stp.cfg @@ -52,8 +52,12 @@ cs7 instance 0 asp asp0 0 2905 m3ua remote-ip 172.18.7.2 + role sg + sctp-role server asp asp-s-0 0 14001 sua remote-ip 172.18.7.3 + role sg + sctp-role server as as0 m3ua asp asp0 routing-key 23 0.2.7 diff --git a/nplab-sua-test/osmo-stp.cfg b/nplab-sua-test/osmo-stp.cfg index 4adc139..b9e754b 100644 --- a/nplab-sua-test/osmo-stp.cfg +++ b/nplab-sua-test/osmo-stp.cfg @@ -52,8 +52,12 @@ cs7 instance 0 asp asp0 0 2905 m3ua remote-ip 172.18.6.2 + role sg + sctp-role server asp asp-s-0 0 14001 sua remote-ip 172.18.6.3 + role sg + sctp-role server as as0 m3ua asp asp0 routing-key 23 0.2.7 diff --git a/osmo-stp-latest/osmo-stp.cfg b/osmo-stp-latest/osmo-stp.cfg index c4b7d41..47d05a4 100644 --- a/osmo-stp-latest/osmo-stp.cfg +++ b/osmo-stp-latest/osmo-stp.cfg @@ -30,8 +30,12 @@ cs7 instance 0 asp asp0 0 2905 m3ua remote-ip 172.18.0.2 + role sg + sctp-role server asp asp-s-0 0 14001 sua remote-ip 172.18.0.3 + role sg + sctp-role server as as0 m3ua asp asp0 routing-key 23 0.2.7 diff --git a/osmo-stp-master/osmo-stp.cfg b/osmo-stp-master/osmo-stp.cfg index 09fa036..edce15d 100644 --- a/osmo-stp-master/osmo-stp.cfg +++ b/osmo-stp-master/osmo-stp.cfg @@ -31,8 +31,12 @@ cs7 instance 0 asp asp0 0 2905 m3ua remote-ip 172.18.0.2 + role sg + sctp-role server asp asp-s-0 0 14001 sua remote-ip 172.18.0.3 + role sg + sctp-role server as as0 m3ua asp asp0 routing-key 23 0.2.7 diff --git a/ttcn3-bsc-test-vamos/osmo-stp.cfg b/ttcn3-bsc-test-vamos/osmo-stp.cfg index c254a4a..a6a920b 100644 --- a/ttcn3-bsc-test-vamos/osmo-stp.cfg +++ b/ttcn3-bsc-test-vamos/osmo-stp.cfg @@ -27,6 +27,8 @@ local-ip fd02:db8:31::200 remote-ip 172.18.31.203 remote-ip fd02:db8:31::203 + role sg + sctp-role server as mahlzeit ipa routing-key 0 0.23.4 point-code override dpc 0.23.1 @@ -39,6 +41,8 @@ local-ip fd02:db8:31::200 remote-ip 172.18.31.203 remote-ip fd02:db8:31::203 + role sg + sctp-role server as virt-msc1 m3ua asp virt-msc1-0 routing-key 2 0.0.2 @@ -48,6 +52,8 @@ local-ip fd02:db8:31::200 remote-ip 172.18.31.203 remote-ip fd02:db8:31::203 + role sg + sctp-role server as virt-msc2 m3ua asp virt-msc2-0 routing-key 3 0.0.3 @@ -57,6 +63,8 @@ local-ip fd02:db8:31::200 remote-ip 172.18.31.203 remote-ip fd02:db8:31::203 + role sg + sctp-role server as virt-smlc m3ua asp virt-smlc-0 routing-key 6 0.23.6 diff --git a/ttcn3-bsc-test/osmo-stp.cfg b/ttcn3-bsc-test/osmo-stp.cfg index 9057564..4137373 100644 --- a/ttcn3-bsc-test/osmo-stp.cfg +++ b/ttcn3-bsc-test/osmo-stp.cfg @@ -40,6 +40,8 @@ local-ip fd02:db8:2::200 remote-ip 172.18.2.203 remote-ip fd02:db8:2::203 + role sg + sctp-role server as mahlzeit ipa routing-key 0 0.23.4 point-code override dpc 0.23.1 @@ -52,6 +54,8 @@ local-ip fd02:db8:2::200 remote-ip 172.18.2.203 remote-ip fd02:db8:2::203 + role sg + sctp-role server as virt-msc1 m3ua asp virt-msc1-0 routing-key 2 0.0.2 @@ -61,6 +65,8 @@ local-ip fd02:db8:2::200 remote-ip 172.18.2.203 remote-ip fd02:db8:2::203 + role sg + sctp-role server as virt-msc2 m3ua asp virt-msc2-0 routing-key 3 0.0.3 @@ -70,6 +76,8 @@ local-ip fd02:db8:2::200 remote-ip 172.18.2.203 remote-ip fd02:db8:2::203 + role sg + sctp-role server as virt-smlc m3ua asp virt-smlc-0 routing-key 6 0.23.6 diff --git a/ttcn3-hnbgw-test/osmo-stp.cfg b/ttcn3-hnbgw-test/osmo-stp.cfg index 76685eb..16ab3fa 100644 --- a/ttcn3-hnbgw-test/osmo-stp.cfg +++ b/ttcn3-hnbgw-test/osmo-stp.cfg @@ -34,12 +34,16 @@ asp virt-msc0-0 23905 2905 m3ua local-ip 172.18.35.200 remote-ip 172.18.35.203 + role sg + sctp-role server as virt-msc0 m3ua asp virt-msc0-0 routing-key 1 0.23.4 asp virt-sgsn0-0 23906 2905 m3ua local-ip 172.18.35.200 remote-ip 172.18.35.203 + role sg + sctp-role server as virt-sgsn0 m3ua asp virt-sgsn0-0 routing-key 2 0.23.1 diff --git a/ttcn3-msc-test/osmo-stp.cfg b/ttcn3-msc-test/osmo-stp.cfg index 8e5ad82..a893f4b 100644 --- a/ttcn3-msc-test/osmo-stp.cfg +++ b/ttcn3-msc-test/osmo-stp.cfg @@ -24,6 +24,8 @@ local-ip fd02:db8:20::200 remote-ip 172.18.20.103 remote-ip fd02:db8:20::103 + role sg + sctp-role server as virt-bsc0 m3ua asp virt-bsc0-0 routing-key 0 0.24.1 @@ -32,6 +34,8 @@ local-ip fd02:db8:20::200 remote-ip 172.18.20.103 remote-ip fd02:db8:20::103 + role sg + sctp-role server as virt-bsc1 m3ua asp virt-bsc1-0 routing-key 1 0.24.2 @@ -40,6 +44,8 @@ local-ip fd02:db8:20::200 remote-ip 172.18.20.103 remote-ip fd02:db8:20::103 + role sg + sctp-role server as virt-rnc0 m3ua asp virt-rnc0-0 routing-key 2 0.24.3 diff --git a/ttcn3-sgsn-test/osmo-stp.cfg b/ttcn3-sgsn-test/osmo-stp.cfg index 127a2c5..f1b6f49 100644 --- a/ttcn3-sgsn-test/osmo-stp.cfg +++ b/ttcn3-sgsn-test/osmo-stp.cfg @@ -24,6 +24,8 @@ local-ip fd02:db8:8::200 remote-ip 172.18.8.103 remote-ip fd02:db8:8::103 + role sg + sctp-role server as virt-rnc0 m3ua asp virt-rnc0-0 routing-key 2 0.24.3 diff --git a/ttcn3-smlc-test/osmo-stp.cfg b/ttcn3-smlc-test/osmo-stp.cfg index efe795e..fb87a9c 100644 --- a/ttcn3-smlc-test/osmo-stp.cfg +++ b/ttcn3-smlc-test/osmo-stp.cfg @@ -30,6 +30,8 @@ local-ip fd02:db8:23::200 remote-ip 172.18.23.203 remote-ip fd02:db8:23::203 + role sg + sctp-role server as virt-bsc0 m3ua asp virt-bsc0-0 routing-key 1 0.23.3 diff --git a/ttcn3-stp-test/osmo-stp.cfg b/ttcn3-stp-test/osmo-stp.cfg index e3083da..28e2363 100644 --- a/ttcn3-stp-test/osmo-stp.cfg +++ b/ttcn3-stp-test/osmo-stp.cfg @@ -29,37 +29,43 @@ local-ip fd02:db8:19::200 remote-ip 172.18.19.203 remote-ip fd02:db8:19::203 + role sg + sctp-role server asp asp-receiver0 10000 2905 m3ua local-ip 172.18.19.200 local-ip fd02:db8:19::200 remote-ip 172.18.19.203 remote-ip fd02:db8:19::203 + role sg + sctp-role server asp asp-receiver1 10001 2905 m3ua local-ip 172.18.19.200 local-ip fd02:db8:19::200 remote-ip 172.18.19.203 remote-ip fd02:db8:19::203 + role sg + sctp-role server asp asp-client0 10002 2906 m3ua local-ip 172.18.19.200 local-ip fd02:db8:19::200 remote-ip 172.18.19.203 remote-ip fd02:db8:19::203 - sctp-role client role asp + sctp-role client asp asp-client60-norctx0 11060 2907 m3ua local-ip 172.18.19.200 local-ip fd02:db8:19::200 remote-ip 172.18.19.203 remote-ip fd02:db8:19::203 - sctp-role client role asp + sctp-role client asp asp-client61-norctx0 11061 2907 m3ua local-ip 172.18.19.200 local-ip fd02:db8:19::200 remote-ip 172.18.19.203 remote-ip fd02:db8:19::203 - sctp-role client role asp + sctp-role client as as-sender m3ua asp asp-sender routing-key 1023 23 @@ -90,9 +96,13 @@ asp ipa-asp-loadshare-receiver0 20001 5000 ipa local-ip 172.18.19.200 remote-ip 172.18.19.203 + role sg + sctp-role server asp ipa-asp-loadshare-receiver1 20002 5000 ipa local-ip 172.18.19.200 remote-ip 172.18.19.203 + role sg + sctp-role server as ipa-as-loadshare-receiver ipa traffic-mode loadshare routing-key 0 7 @@ -110,9 +120,13 @@ asp ipa-asp-override-receiver0 20005 5000 ipa local-ip 172.18.19.200 remote-ip 172.18.19.203 + role sg + sctp-role server asp ipa-asp-override-receiver1 20006 5000 ipa local-ip 172.18.19.200 remote-ip 172.18.19.203 + role sg + sctp-role server as ipa-as-override-receiver ipa traffic-mode override routing-key 0 8 -- To view, visit https://gerrit.osmocom.org/c/docker-playground/+/33544 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: docker-playground Gerrit-Branch: master Gerrit-Change-Id: Ia495bc3c5dd4421e5730c74b2f5dc4e4cdc1a673 Gerrit-Change-Number: 33544 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in osmo-pcu[master]: gprs_ms: Constify variable in ms_is_reachable_for_dl_ass()
by pespin 05 Jul '23

05 Jul '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33574 ) Change subject: gprs_ms: Constify variable in ms_is_reachable_for_dl_ass() ...................................................................... gprs_ms: Constify variable in ms_is_reachable_for_dl_ass() Change-Id: If37907b2f75b61b3243b9d8499bb0cf5c02c28c1 --- M src/gprs_ms.c 1 file changed, 11 insertions(+), 2 deletions(-) Approvals: fixeria: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/gprs_ms.c b/src/gprs_ms.c index 6e6390b..5890d31 100644 --- a/src/gprs_ms.c +++ b/src/gprs_ms.c @@ -1093,7 +1093,7 @@ /* Can we get to send a DL TBF ass to the MS? */ static bool ms_is_reachable_for_dl_ass(const struct GprsMs *ms) { - struct gprs_rlcmac_ul_tbf *ul_tbf = ms_ul_tbf(ms); + const struct gprs_rlcmac_ul_tbf *ul_tbf = ms_ul_tbf(ms); /* This function assumes it is called when no DL TBF is present */ OSMO_ASSERT(!ms_dl_tbf(ms)); @@ -1115,7 +1115,7 @@ return true; if (ul_tbf_contention_resolution_done(ul_tbf) && !tbf_ul_ack_waiting_cnf_final_ack(ul_tbf) && - tbf_state(ul_tbf_as_tbf(ul_tbf)) != TBF_ST_RELEASING) + tbf_state(ul_tbf_as_tbf_const(ul_tbf)) != TBF_ST_RELEASING) return true; return false; -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33574 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: If37907b2f75b61b3243b9d8499bb0cf5c02c28c1 Gerrit-Change-Number: 33574 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in osmo-pcu[master]: gprs_ms: Update assert condition
by pespin 05 Jul '23

05 Jul '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33575 ) Change subject: gprs_ms: Update assert condition ...................................................................... gprs_ms: Update assert condition A recent commit adding TBF_ST_WAIT_REUSE_TFI state updated ms_append_llc_dl_data() to attempt creating a new DL TBF while in that state, but forgot to update the assert checking for the conditions in ms_is_reachable_for_dl_ass(). Related: OS#6084 Fixes: 40a297f3b0c8e1670d46a4974750dd3335bc7885 Change-Id: I3d51e909c9a9f688b7f4425a5ba319d183c71d1f --- M src/gprs_ms.c 1 file changed, 21 insertions(+), 2 deletions(-) Approvals: fixeria: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/gprs_ms.c b/src/gprs_ms.c index 5890d31..7fa3fc0 100644 --- a/src/gprs_ms.c +++ b/src/gprs_ms.c @@ -1093,10 +1093,13 @@ /* Can we get to send a DL TBF ass to the MS? */ static bool ms_is_reachable_for_dl_ass(const struct GprsMs *ms) { + const struct gprs_rlcmac_dl_tbf *dl_tbf = ms_dl_tbf(ms); const struct gprs_rlcmac_ul_tbf *ul_tbf = ms_ul_tbf(ms); - /* This function assumes it is called when no DL TBF is present */ - OSMO_ASSERT(!ms_dl_tbf(ms)); + /* This function assumes it is called when no DL TBF is present, or + * alternatively if it's not really in use by the MS (TBF_ST_WAIT_REUSE_TFI) */ + OSMO_ASSERT(!dl_tbf || + tbf_state(dl_tbf_as_tbf_const(dl_tbf)) == TBF_ST_WAIT_REUSE_TFI); /* 3GPP TS 44.060 sec 7.1.3.1 Initiation of the Packet resource request procedure: * "Furthermore, the mobile station shall not respond to PACKET DOWNLINK ASSIGNMENT -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33575 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I3d51e909c9a9f688b7f4425a5ba319d183c71d1f Gerrit-Change-Number: 33575 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in osmo-pcu[master]: pcuif: Log read() error cause
by pespin 05 Jul '23

05 Jul '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33573 ) Change subject: pcuif: Log read() error cause ...................................................................... pcuif: Log read() error cause Change-Id: Ifdc46a228752452c8d403f2d49f4bac23c450b4d --- M src/osmobts_sock.c 1 file changed, 10 insertions(+), 0 deletions(-) Approvals: fixeria: Looks good to me, but someone else must approve Jenkins Builder: Verified osmith: Looks good to me, approved diff --git a/src/osmobts_sock.c b/src/osmobts_sock.c index c4a042f..c77605f 100644 --- a/src/osmobts_sock.c +++ b/src/osmobts_sock.c @@ -138,6 +138,7 @@ if (rc < 0 && errno == EAGAIN) return 0; /* Try again later */ if (rc <= 0) { + LOGP(DL1IF, LOGL_ERROR, "%s: recv() failed with rc=%d errno=%d\n", __func__, rc, errno); pcu_sock_close(1); return -EIO; } -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33573 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ifdc46a228752452c8d403f2d49f4bac23c450b4d Gerrit-Change-Number: 33573 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> 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-MessageType: merged

1 0

[S] Change in osmo-trx[master]: ms: new bb, no fn delay
by Hoernchen 05 Jul '23

05 Jul '23

Hoernchen has abandoned this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/33560 ) Change subject: ms: new bb, no fn delay ...................................................................... Abandoned -- To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/33560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-Change-Id: I506e9f841646373abdd17278ee8b48a01dc3725f Gerrit-Change-Number: 33560 Gerrit-PatchSet: 3 Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: abandon

1 0

[S] Change in osmo-trx[master]: ms: new bb split
by Hoernchen 05 Jul '23

05 Jul '23

Hoernchen has abandoned this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/33559 ) Change subject: ms: new bb split ...................................................................... Abandoned -- To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/33559 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-Change-Id: If3a47105e777a86a311646b4f30cb756e046dc56 Gerrit-Change-Number: 33559 Gerrit-PatchSet: 3 Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: abandon

1 0

[S] Change in osmo-pcu[master]: gprs_ms: Update assert condition
by laforge 05 Jul '23

05 Jul '23

Attention is currently required from: pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33575 ) Change subject: gprs_ms: Update assert condition ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33575 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I3d51e909c9a9f688b7f4425a5ba319d183c71d1f Gerrit-Change-Number: 33575 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 05 Jul 2023 13:40:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-msc[master]: ASCI: Receive messages from MSC-A role related to VGCS/VBS
by laforge 05 Jul '23

05 Jul '23

Attention is currently required from: jolly. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33515 ) Change subject: ASCI: Receive messages from MSC-A role related to VGCS/VBS ...................................................................... Patch Set 12: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33515 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: Ie68eedb8fcb064a55cd71b58630d7a8c8b5f29ad Gerrit-Change-Number: 33515 Gerrit-PatchSet: 12 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Comment-Date: Wed, 05 Jul 2023 13:40:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in osmo-msc[master]: ASCI: Add VTY to configure GCR (Group Call Register)
by laforge 05 Jul '23

05 Jul '23

Attention is currently required from: jolly. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33512 ) Change subject: ASCI: Add VTY to configure GCR (Group Call Register) ...................................................................... Patch Set 12: (3 comments) File src/libmsc/asci_vty.c: https://gerrit.osmocom.org/c/osmo-msc/+/33512/comment/aa36a2b0_914b9692 PS10, Line 38: DEFUN(vgcs_call, vgcs_call_cmd, > This is used to initiate a voice group call from the VTY. […] Done https://gerrit.osmocom.org/c/osmo-msc/+/33512/comment/55d175ba_6c2fdb5c PS10, Line 219: "mute-talker", "Mute talker's downlink") > This is not a dispatcher function. Unmuting talker is used for testing a call with a single phone. […] if it's only for testing then it probably should be HIDDEN and maybe also not saved in the config file? https://gerrit.osmocom.org/c/osmo-msc/+/33512/comment/ce900301_0e7429cc PS10, Line 261: "no cell POINT_CODE [<0-65535>]", NO_STR "Remove BSS/cell from current group\n" PC_ID_STR) > It is default 3.8.8 format. I changed the description accordingly. as all our libosmo-sigtran code has user-configurable point code format, I think the same point code format should be used here. Otherwise it's inconsistent to the user. The user doesn't know whihc part of the config is parsed by a library or the application. He'd just be presented with s ome config lines that use his specified format, while others require a hard-coded format. -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33512 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I5bd034a62fc8b483f550d29103c2f7587198f590 Gerrit-Change-Number: 33512 Gerrit-PatchSet: 12 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Comment-Date: Wed, 05 Jul 2023 13:39:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: jolly <andreas(a)eversberg.eu> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

1 0

[XL] Change in osmo-msc[master]: ASCI: Add call control for VGCS/VBS
by laforge 05 Jul '23

05 Jul '23

Attention is currently required from: jolly. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33511 ) Change subject: ASCI: Add call control for VGCS/VBS ...................................................................... Patch Set 10: (15 comments) File src/libmsc/vgcs_fsm.c: https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/5f7b6d12_9b45ad97 PS10, Line 123: tatic __attribute__((constructor)) void vgcs_bcc_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_bcc_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_gcc_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_gcc_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_bss_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_bss_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_cell_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_cell_fsm) == 0); : } : : static __attribute__((constructor)) void vgcs_mgw_ep_fsm_init(void) : { : OSMO_ASSERT(osmo_fsm_register(&vgcs_mgw_ep_fsm) == 0); : } is there a reason to have separate constructor symbols for this? Why not have one consturctor function which has N OSMO_ASSERT statements? https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/5676916c_34b24ae6 PS10, Line 152: sprintf(string, "%08u", callref); yes, 8 decimal digits plus zero termination is shorter than 16 characters, but I'd say as a matter of good practice we should still use snprintf. This makes it easier for somebody else to to audit the code, where every sprintf requires auditing if it's really ok, with snprintf you can just ignore it from manual audit/inspection. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/f5c4c9e6_54602382 PS10, Line 222: if (with_prio) also here I would first build the 32bit value as a stack variable of uint32_t type and use msgb_put_u32() to avoid having to type-cast around and using the third byte of an uint32.... https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/96efd94e_f92b21a9 PS10, Line 232: int _ie_invalid(void) are those (_add_cause_ie, _add_callref_ie, _msg_too_short, _ie_invalid) really intended to be exported (non-static) functions? IF yes, then their name should be expanded to reflect they relate to ASCI as the name is very geneirc. If not, then they should be static. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/436e05d4_dca1bc9d PS10, Line 249: e = msgb_put(msg, 1) you can first buld the uint8_t as a local variable and then use msgb_put_u8(msg, value), avoiding a pointer to an array and indexing only the 0th element in it. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/48de5e4a_926d7567 PS10, Line 265: #if not sure why this is commented out? because there are no users yet and it's a static function? IF there is a good reason to do it like this, then add a comment as explanation, please. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/4915c3cf_455819db PS10, Line 314: payload_len as you're decrmenting this variable, I thing I'd call it remaining_len or remaining_payload_len or something like that. payload_len sounds like the total length of the payloda field https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/1d80b9d4_bcf8d0f6 PS10, Line 323: [0] + 1; I don't have an immediate better solution, but I really think ti is very hard to read/follow and particularly had to say if it is correct, contains some overflows, .... You're basically incrementing the ie pointer as you go, but also deducting the payload_len pointer, both by the same amount, causing duplicate sub-expressions like "ie[0] + 1" which all must be consistent. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/7c636d95_5af9fece PS10, Line 337: ie[0] + here I think you're trusting ie[0] blindly, without having done much checking on it? probably osmo_mobile_identity_decode() imlpicitly trusts it as it is used as input variable. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/33b80962_fc054624 PS10, Line 346: ie += 4; in each of those blocks we have a magic value (length) that is used three times and which must be consistent. 4 here in this example. I think there must be a way without all those copies of the same value. Like some kind of helper functions, or in the worst case macros? https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/a6d317a3_034b52be PS10, Line 391: ie[0] = (da << 3) again I'd put the value together in a uint8_t local variable and then msgb_put_u8() it aftrewards. This way we're not de-refernecing the zero'th element in an aray of 1, which makes it harder to read (and one might assume the array might have more elements, ... https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/5c2a76a1_90ecbadf PS10, Line 416: if (ie[3] & 0x10) { same comments as other decoders above. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/475f3568_92b349ae PS10, Line 604: osmo_fsm_inst_free(mgw->fi); we typically use the goto err_* label construct here to not have to repeat all the various free functions in every case. https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/e7774254_19940618 PS10, Line 694: return -EINVAL; here we return -EINVAL without talloc_free etc? https://gerrit.osmocom.org/c/osmo-msc/+/33511/comment/6dbde9df_e5b6777e PS10, Line 1279: int gsm44068_rcv_bcc_gcc(struct msc_a *msc_a, struct gsm_trans *trans, struct msgb *msg) this function is quite long. Even the variable list alone fills half a page on soem screen heights. That's usually a sign that it should be pslit up into sub-functions. -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33511 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I9947403fde8212b66758104443c60aaacc8b1e7b Gerrit-Change-Number: 33511 Gerrit-PatchSet: 10 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: dexter <pmaier(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Comment-Date: Wed, 05 Jul 2023 13:36:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0