Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
fixeria has abandoned this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Abandoned -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-MessageType: abandon
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 3: > Patch Set 3: > > I think we should rather tag a new libosmocore and update the dependency > requirement everywhere. I'm not against this single fix, but I guess we'd > have to fix lots of places if we want to avoid any trouble with old > libosmocore? Agree with Harald, same reasons he explained. So I'm strongly against merging this change. -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 16 Jul 2019 18:25:43 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 3: I think we should rather tag a new libosmocore and update the dependency requirement everywhere. I'm not against this single fix, but I guess we'd have to fix lots of places if we want to avoid any trouble with old libosmocore? -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 16 Jul 2019 04:10:18 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 3: @pespin, I think it still makes sense to merge this change. Since the matching problem has been solved in the recent libosmovty, it may be useful for those users who are using release version of libosmocore, or an older version. -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 08 Jul 2019 12:00:12 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-Comment-Date: Thu, 06 Jun 2019 20:30:23 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 2: > I created https://osmocom.org/issues/4045 to track this issue and a fix for > libosmovty. Thanks. Please also see: https://gerrit.osmocom.org/#/c/libosmocore/+/14307/ -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 05 Jun 2019 11:16:57 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 2: I created https://osmocom.org/issues/4045 to track this issue and a fix for libosmovty. -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 05 Jun 2019 10:24:35 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 2: Code-Review-1 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 05 Jun 2019 09:53:57 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-CC: pespin Gerrit-Comment-Date: Tue, 04 Jun 2019 21:31:12 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/14195/1//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/#/c/14195/1//COMMIT_MSG@22 PS1, Line 22: auth-policy c > Yes, the VTY parser would pass any of those variations. […] Sure, matching like that is fine and expected. What I am saying is once matched, strings passed as argv[*] to VTY apps functions should be "complete" commands, aka "accept" instead of "acc" even if you typed "acc". This patch imho is just a quick fix in some place, but I bet we have similar issues in lots of other places, so we should fix it in libosmocore code instead of here. Otherwise having to check for all possible argv strings in all commands becomes unmanageable. -- To view, visit https://gerrit.osmocom.org/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 1 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Pau Espin Pedrol Gerrit-Comment-Date: Mon, 27 May 2019 10:00:29 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/14195/1//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/#/c/14195/1//COMMIT_MSG@22 PS1, Line 22: auth-policy c > So you say in this case argv[0] is "c" instead of "closed"? That's IMHO wrong > and should be fixed in […] Yes, the VTY parser would pass any of those variations. I guess the problem is that we are doing: strncmp( choice, input, strlen(input) ) somewhere in the VTY code, so for example: strncmp( "accept-all", "accept", 6 ) == 0 strncmp( "accept-all", "acc", 3 ) == 0 strncmp( "accept-all", "a", 1 ) == 0 strncmp( "accept-all", "", 0 ) == 0 Also, that's probably why you can type: - "en" instead of "enable", - "subscr" instead of "subscriber". Unfortunately, I am not familiar with the internals of libosmovty, so for now it makes sense at least to avoid segfault'ing here. -- To view, visit https://gerrit.osmocom.org/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 1 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Vadim Yanitskiy Gerrit-CC: Pau Espin Pedrol Gerrit-Comment-Date: Mon, 27 May 2019 08:54:15 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/14195 ) Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/14195/1//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/#/c/14195/1//COMMIT_MSG@22 PS1, Line 22: auth-policy c So you say in this case argv[0] is "c" instead of "closed"? That's IMHO wrong and should be fixed in VTY libosmocore code. -- To view, visit https://gerrit.osmocom.org/14195 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8 Gerrit-Change-Number: 14195 Gerrit-PatchSet: 1 Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Pau Espin Pedrol Gerrit-Comment-Date: Mon, 27 May 2019 08:31:55 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-sgsn[master]: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
Vadim Yanitskiy has uploaded this change for review. (
https://gerrit.osmocom.org/14195
Change subject: osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
..
osmo-sgsn: get rid of OSMO_ASSERT() in 'auth-policy' handler
For some reason, libosmovty does accept incomplete commands
if a command definition contains 'choice' statements.
For example, the following command definition:
auth-policy (accept-all|closed|acl-only|remote)
actually permits the following variations:
auth-policy accept-all
auth-policy accept
auth-policy acl
auth-policy re
auth-policy c
so in case of such incomplete input, get_string_value() would fail
to find the corresponding enum value, and the whole process would
crash. That's not what a regular fun of the TAB-completion would
expect, right?
Instead of assert()ing the result of get_string_value(), let's
rather print a warning and return CMD_ERR_INCOMPLETE.
Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8
---
M src/gprs/sgsn_vty.c
1 file changed, 7 insertions(+), 3 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/95/14195/1
diff --git a/src/gprs/sgsn_vty.c b/src/gprs/sgsn_vty.c
index 9155441..969156f 100644
--- a/src/gprs/sgsn_vty.c
+++ b/src/gprs/sgsn_vty.c
@@ -725,9 +725,13 @@
"Accept only subscribers in the ACL\n"
"Use remote subscription data only (HLR)\n")
{
- int val = get_string_value(sgsn_auth_pol_strs, argv[0]);
- OSMO_ASSERT(val >= SGSN_AUTH_POLICY_OPEN && val <=
SGSN_AUTH_POLICY_REMOTE);
- g_cfg->auth_policy = val;
+ int val;
+
+ val = get_string_value(sgsn_auth_pol_strs, argv[0]);
+ if (val < 0)
+ return CMD_ERR_INCOMPLETE;
+
+ g_cfg->auth_policy = (enum sgsn_auth_policy) val;
g_cfg->require_update_location = (val == SGSN_AUTH_POLICY_REMOTE);
/* Authentication is not possible without HLR */
--
To view, visit https://gerrit.osmocom.org/14195
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If9b0c0d031477ca87786aab5c269d00748e896c8
Gerrit-Change-Number: 14195
Gerrit-PatchSet: 1
Gerrit-Owner: Vadim Yanitskiy
