Change in ...osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
laforge has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/osmo-pcu/+/14171 )
Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi
instance
..
gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
The instance bssgp_nsi is a global instance to be used by all
NS related functions. Previous the PCU allocated and initialized
the bssgp_nsi instance when (re-)connecting and freeing on disconnect.
The problem of the implicit initialisation is gprs_ns_vty_init(bssgp_nsi).
All vty init functions must be called before the configuration is read,
otherwise a previous vty written configuration is invalid.
Furthermore the vty modifications to the `ns` object were lost when the PCU has
to
reconnect to the SGSN.
Fixes: OS#4024
Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf
---
M src/gprs_bssgp_pcu.cpp
M src/pcu_main.cpp
M tests/emu/pcu_emu.cpp
M tests/tbf/TbfTest.cpp
4 files changed, 49 insertions(+), 17 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp
index 50f10db..22abc6b 100644
--- a/src/gprs_bssgp_pcu.cpp
+++ b/src/gprs_bssgp_pcu.cpp
@@ -875,11 +875,6 @@
{
struct gprs_nsvc *nsvc2;
- if (!bssgp_nsi) {
- LOGP(DBSSGP, LOGL_ERROR, "NS instance does not exist\n");
- return -EINVAL;
- }
-
if (nsvc != the_pcu.nsvc) {
LOGP(DBSSGP, LOGL_ERROR, "NSVC is invalid\n");
return -EBADF;
@@ -913,12 +908,6 @@
the_pcu.bts = bts;
- bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
- if (!bssgp_nsi) {
- LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
- return NULL;
- }
- gprs_ns_vty_init(bssgp_nsi);
/* don't specify remote IP/port if SNS dialect is in use; Doing so would
* issue a connect() on the socket, which prevents us to dynamically
communicate
* with any number of IP-SNS endpoints on the SGSN side */
@@ -930,8 +919,7 @@
rc = gprs_ns_nsip_listen(bssgp_nsi);
if (rc < 0) {
LOGP(DBSSGP, LOGL_ERROR, "Failed to create socket\n");
- gprs_ns_destroy(bssgp_nsi);
- bssgp_nsi = NULL;
+ gprs_ns_close(bssgp_nsi);
return NULL;
}
@@ -945,8 +933,7 @@
the_pcu.nsvc = gprs_ns_nsip_connect(bssgp_nsi, , nsei,
nsvci);
if (!the_pcu.nsvc) {
LOGP(DBSSGP, LOGL_ERROR, "Failed to create NSVCt\n");
- gprs_ns_destroy(bssgp_nsi);
- bssgp_nsi = NULL;
+ gprs_ns_close(bssgp_nsi);
return NULL;
}
@@ -954,8 +941,7 @@
if (!the_pcu.bctx) {
LOGP(DBSSGP, LOGL_ERROR, "Failed to create BSSGP context\n");
the_pcu.nsvc = NULL;
- gprs_ns_destroy(bssgp_nsi);
- bssgp_nsi = NULL;
+ gprs_ns_close(bssgp_nsi);
return NULL;
}
the_pcu.bctx->ra_id.mcc = spoof_mcc ? : mcc;
diff --git a/src/pcu_main.cpp b/src/pcu_main.cpp
index 1003e3c..fa075cd 100644
--- a/src/pcu_main.cpp
+++ b/src/pcu_main.cpp
@@ -33,6 +33,8 @@
#include
#include
#include
+#include "gprs_bssgp_pcu.h"
+
extern "C" {
#include "pcu_vty.h"
#include
@@ -291,6 +293,13 @@
else
fprintf(stderr, "Failed to initialize GSMTAP for %s\n",
gsmtap_addr);
+ bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
+ if (!bssgp_nsi) {
+ LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
+ exit(1);
+ }
+ gprs_ns_vty_init(bssgp_nsi);
+
rc = vty_read_config_file(config_file, NULL);
if (rc < 0 && config_given) {
fprintf(stderr, "Failed to parse the config file: '%s'\n",
diff --git a/tests/emu/pcu_emu.cpp b/tests/emu/pcu_emu.cpp
index 354a328..e0190bf 100644
--- a/tests/emu/pcu_emu.cpp
+++ b/tests/emu/pcu_emu.cpp
@@ -113,6 +113,13 @@
msgb_talloc_ctx_init(tall_pcu_ctx, 0);
osmo_init_logging2(tall_pcu_ctx, _log_info);
+
+ bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
+ if (!bssgp_nsi) {
+ LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
+ abort();
+ }
+
vty_init(_vty_info);
pcu_vty_init(_log_info);
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 151e7fa..675cf89 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -447,6 +447,12 @@
printf("=== start %s ===\n", __func__);
+ bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
+ if (!bssgp_nsi) {
+ LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
+
Change in ...osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 2 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Comment-Date: Thu, 13 Jun 2019 15:35:51 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 2 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Comment-Date: Fri, 07 Jun 2019 00:32:31 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 2: -Code-Review (1 comment) https://gerrit.osmocom.org/#/c/14171/1/src/gprs_bssgp_pcu.cpp File src/gprs_bssgp_pcu.cpp: https://gerrit.osmocom.org/#/c/14171/1/src/gprs_bssgp_pcu.cpp@878 PS1, Line 878: OSMO_ASSERT(bssgp_nsi); > I'll drop this assert, as all calls should be guaranteed with an bssgp_nsi Done -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 2 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Comment-Date: Fri, 07 Jun 2019 00:04:06 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: lynxis lazus Gerrit-MessageType: comment
Change in ...osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
Hello fixeria, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/14171 to look at the new patch set (#2). Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance The instance bssgp_nsi is a global instance to be used by all NS related functions. Previous the PCU allocated and initialized the bssgp_nsi instance when (re-)connecting and freeing on disconnect. The problem of the implicit initialisation is gprs_ns_vty_init(bssgp_nsi). All vty init functions must be called before the configuration is read, otherwise a previous vty written configuration is invalid. Furthermore the vty modifications to the `ns` object were lost when the PCU has to reconnect to the SGSN. Fixes: OS#4024 Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf --- M src/gprs_bssgp_pcu.cpp M src/pcu_main.cpp M tests/emu/pcu_emu.cpp M tests/tbf/TbfTest.cpp 4 files changed, 49 insertions(+), 17 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/71/14171/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 2 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-MessageType: newpatchset
Change in osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 1: Code-Review-2 I'll drop the OSMO_ASSERT + free the bssgp_nsi in pcu_main.c -- To view, visit https://gerrit.osmocom.org/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 1 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: lynxis lazus Gerrit-Comment-Date: Sat, 25 May 2019 17:16:21 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 1: Code-Review-1 (1 comment) https://gerrit.osmocom.org/#/c/14171/1/src/gprs_bssgp_pcu.cpp File src/gprs_bssgp_pcu.cpp: https://gerrit.osmocom.org/#/c/14171/1/src/gprs_bssgp_pcu.cpp@878 PS1, Line 878: OSMO_ASSERT(bssgp_nsi); I'll drop this assert, as all calls should be guaranteed with an bssgp_nsi -- To view, visit https://gerrit.osmocom.org/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 1 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Reviewer: lynxis lazus Gerrit-Comment-Date: Sat, 25 May 2019 17:15:51 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 1: Code-Review+2 (1 comment) Finally! Thanks a lot for looking into this! A few days ago I faced this bug, when you can 'write' the VTY configuration, but then reading fails. https://gerrit.osmocom.org/#/c/14171/1/src/gprs_bssgp_pcu.cpp File src/gprs_bssgp_pcu.cpp: https://gerrit.osmocom.org/#/c/14171/1/src/gprs_bssgp_pcu.cpp@906 PS1, Line 906: Makes sense to OSMO_ASSERT(!bssgp_nsi) here. -- To view, visit https://gerrit.osmocom.org/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 1 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-Comment-Date: Sat, 25 May 2019 11:19:10 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/14171 ) Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/14171 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf Gerrit-Change-Number: 14171 Gerrit-PatchSet: 1 Gerrit-Owner: lynxis lazus Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Comment-Date: Sat, 25 May 2019 09:09:26 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-pcu[master]: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
lynxis lazus has uploaded this change for review. (
https://gerrit.osmocom.org/14171
Change subject: gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi
instance
..
gprs_bssgp_pcu: explicit allocate & initialize bssgp_nsi instance
The instance bssgp_nsi is a global instance to be used by all
NS related functions. Previous the PCU allocated and initialized
the bssgp_nsi instance when (re-)connecting and freeing on disconnect.
The problem of the implicit initialisation is gprs_ns_vty_init(bssgp_nsi).
All vty init functions must be called before the configuration is read,
otherwise a previous vty written configuration is invalid.
Furthermore the vty modifications to the `ns` object were lost when the PCU has
to
reconnect to the SGSN.
Fixes: OS#4024
Change-Id: I2aa53ea54e9352577f6280ad7b9d1d9da9f57eaf
---
M src/gprs_bssgp_pcu.cpp
M src/pcu_main.cpp
M tests/emu/pcu_emu.cpp
M tests/tbf/TbfTest.cpp
4 files changed, 50 insertions(+), 16 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/71/14171/1
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp
index 50f10db..b2d9d74 100644
--- a/src/gprs_bssgp_pcu.cpp
+++ b/src/gprs_bssgp_pcu.cpp
@@ -875,10 +875,7 @@
{
struct gprs_nsvc *nsvc2;
- if (!bssgp_nsi) {
- LOGP(DBSSGP, LOGL_ERROR, "NS instance does not exist\n");
- return -EINVAL;
- }
+ OSMO_ASSERT(bssgp_nsi);
if (nsvc != the_pcu.nsvc) {
LOGP(DBSSGP, LOGL_ERROR, "NSVC is invalid\n");
@@ -913,12 +910,6 @@
the_pcu.bts = bts;
- bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
- if (!bssgp_nsi) {
- LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
- return NULL;
- }
- gprs_ns_vty_init(bssgp_nsi);
/* don't specify remote IP/port if SNS dialect is in use; Doing so would
* issue a connect() on the socket, which prevents us to dynamically
communicate
* with any number of IP-SNS endpoints on the SGSN side */
@@ -930,8 +921,7 @@
rc = gprs_ns_nsip_listen(bssgp_nsi);
if (rc < 0) {
LOGP(DBSSGP, LOGL_ERROR, "Failed to create socket\n");
- gprs_ns_destroy(bssgp_nsi);
- bssgp_nsi = NULL;
+ gprs_ns_close(bssgp_nsi);
return NULL;
}
@@ -945,8 +935,7 @@
the_pcu.nsvc = gprs_ns_nsip_connect(bssgp_nsi, , nsei,
nsvci);
if (!the_pcu.nsvc) {
LOGP(DBSSGP, LOGL_ERROR, "Failed to create NSVCt\n");
- gprs_ns_destroy(bssgp_nsi);
- bssgp_nsi = NULL;
+ gprs_ns_close(bssgp_nsi);
return NULL;
}
@@ -954,8 +943,7 @@
if (!the_pcu.bctx) {
LOGP(DBSSGP, LOGL_ERROR, "Failed to create BSSGP context\n");
the_pcu.nsvc = NULL;
- gprs_ns_destroy(bssgp_nsi);
- bssgp_nsi = NULL;
+ gprs_ns_close(bssgp_nsi);
return NULL;
}
the_pcu.bctx->ra_id.mcc = spoof_mcc ? : mcc;
diff --git a/src/pcu_main.cpp b/src/pcu_main.cpp
index 1003e3c..fa075cd 100644
--- a/src/pcu_main.cpp
+++ b/src/pcu_main.cpp
@@ -33,6 +33,8 @@
#include
#include
#include
+#include "gprs_bssgp_pcu.h"
+
extern "C" {
#include "pcu_vty.h"
#include
@@ -291,6 +293,13 @@
else
fprintf(stderr, "Failed to initialize GSMTAP for %s\n",
gsmtap_addr);
+ bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
+ if (!bssgp_nsi) {
+ LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
+ exit(1);
+ }
+ gprs_ns_vty_init(bssgp_nsi);
+
rc = vty_read_config_file(config_file, NULL);
if (rc < 0 && config_given) {
fprintf(stderr, "Failed to parse the config file: '%s'\n",
diff --git a/tests/emu/pcu_emu.cpp b/tests/emu/pcu_emu.cpp
index 354a328..e0190bf 100644
--- a/tests/emu/pcu_emu.cpp
+++ b/tests/emu/pcu_emu.cpp
@@ -113,6 +113,13 @@
msgb_talloc_ctx_init(tall_pcu_ctx, 0);
osmo_init_logging2(tall_pcu_ctx, _log_info);
+
+ bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
+ if (!bssgp_nsi) {
+ LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
+ abort();
+ }
+
vty_init(_vty_info);
pcu_vty_init(_log_info);
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 151e7fa..675cf89 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -447,6 +447,12 @@
printf("=== start %s ===\n", __func__);
+ bssgp_nsi = gprs_ns_instantiate(_bssgp_ns_cb, tall_pcu_ctx);
+ if (!bssgp_nsi) {
+ LOGP(DBSSGP, LOGL_ERROR, "Failed to create NS instance\n");
+ abort();
+ }
+
bts = the_bts.bts_data();
setup_bts(_bts,
