diff mbox

Change in openbsc[master]: Make extending subscriber creation easier

Message ID gerrit.1462958673080.I3b10a9a764fd3a7bb96717a990e52caae16266da@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 11, 2016, 9:24 a.m. UTC 
From Max <msuraev@sysmocom.de>:

Max has uploaded a new change for review.

  https://gerrit.osmocom.org/42

Change subject: Make extending subscriber creation easier
......................................................................

Make extending subscriber creation easier

* rename variable controlling subscriber creation
* use enum for subscriber creation policy
* move check for subscriber creation policy into separate static
  function

Change-Id: I3b10a9a764fd3a7bb96717a990e52caae16266da
---
M openbsc/include/openbsc/gsm_data.h
M openbsc/src/libbsc/net_init.c
M openbsc/src/libmsc/gsm_04_08.c
M openbsc/src/libmsc/vty_interface_layer3.c
4 files changed, 23 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/42/42/1

Comments

gerrit-no-reply@lists.osmocom.org May 11, 2016, 10:17 a.m. UTC | #1 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Make extending subscriber creation easier
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/42/1/openbsc/src/libmsc/vty_interface_layer3.c
File openbsc/src/libmsc/vty_interface_layer3.c:

Line 1075: 		gsmnet->subscr_creation_mode ? "" : "no ", VTY_NEWLINE);
Will you be following-up with another patch that allows us to enable GSM_SUBSCR_CREAT_W_NO_EXT mode?  Right now the enum is introduced, but no way to activate it?  If you have a follow-up, keep this patch with the new enum value, it doesn't hurt.
gerrit-no-reply@lists.osmocom.org May 11, 2016, 10:21 a.m. UTC | #2 
From Max <msuraev@sysmocom.de>:

Max has posted comments on this change.

Change subject: Make extending subscriber creation easier
......................................................................


Patch Set 1:

Yes, definitely, there will be follow-ups. I'm re-sending patch series for imsi authorization and subscribers with no random extension from pre-gerrit time. Both are touching the same code paths. I'm also incorporating feedback received in ML.
gerrit-no-reply@lists.osmocom.org May 11, 2016, 11:10 a.m. UTC | #3 
From Neels Hofmeyr <nhofmeyr@sysmocom.de>:

Neels Hofmeyr has posted comments on this change.

Change subject: Make extending subscriber creation easier
......................................................................


Patch Set 1: Code-Review+1

(2 comments)

maybe tweak cosmetics, and what hwelte said (vty)

https://gerrit.osmocom.org/#/c/42/1/openbsc/src/libmsc/gsm_04_08.c
File openbsc/src/libmsc/gsm_04_08.c:

Line 513: 	if (GSM_SUBSCR_DONT_CREATE == net->subscr_creation_mode)
yoda conditional


Line 652: 			subscr = subscr_create(	bts->network, mi_string);
whitespace "(\t"
gerrit-no-reply@lists.osmocom.org May 11, 2016, 2:14 p.m. UTC | #4 
From Max <msuraev@sysmocom.de>:

Hello Neels Hofmeyr,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/42

to look at the new patch set (#2).

Change subject: Make extending subscriber creation easier
......................................................................

Make extending subscriber creation easier

* rename variable controlling subscriber creation
* use enum for subscriber creation policy
* move check for subscriber creation policy into separate static
  function

Change-Id: I3b10a9a764fd3a7bb96717a990e52caae16266da
---
M openbsc/include/openbsc/gsm_data.h
M openbsc/src/libbsc/net_init.c
M openbsc/src/libmsc/gsm_04_08.c
M openbsc/src/libmsc/vty_interface_layer3.c
4 files changed, 23 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/42/42/2
gerrit-no-reply@lists.osmocom.org May 12, 2016, 6:01 p.m. UTC | #5 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Make extending subscriber creation easier
......................................................................


Patch Set 3: Code-Review+2
gerrit-no-reply@lists.osmocom.org May 17, 2016, 4:11 p.m. UTC | #6 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: Make extending subscriber creation easier
......................................................................


Patch Set 3: Code-Review-1

(1 comment)

https://gerrit.osmocom.org/#/c/42/3/openbsc/src/libmsc/gsm_04_08.c
File openbsc/src/libmsc/gsm_04_08.c:

PS3, Line 652: subscr
still coding style issue with {} for a single line.
gerrit-no-reply@lists.osmocom.org May 17, 2016, 4:20 p.m. UTC | #7 
From Max <msuraev@sysmocom.de>:

Hello Neels Hofmeyr, Harald Welte, Jenkins Builder, Holger Freyther,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/42

to look at the new patch set (#4).

Change subject: Make extending subscriber creation easier
......................................................................

Make extending subscriber creation easier

* rename variable controlling subscriber creation
* use enum for subscriber creation policy
* move check for subscriber creation policy into separate static
  function

Change-Id: I3b10a9a764fd3a7bb96717a990e52caae16266da
---
M openbsc/include/openbsc/gsm_data.h
M openbsc/src/libbsc/net_init.c
M openbsc/src/libmsc/gsm_04_08.c
M openbsc/src/libmsc/vty_interface_layer3.c
4 files changed, 24 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/42/42/4
gerrit-no-reply@lists.osmocom.org May 19, 2016, 6:53 a.m. UTC | #8 
Patch Set 5:

(1 comment)

Last comment about the vty write but we can move to the follow up patch soon. Is there a ticket reference as well?

https://gerrit.osmocom.org/#/c/42/5/openbsc/src/libmsc/vty_interface_layer3.c
File openbsc/src/libmsc/vty_interface_layer3.c:

Line 1075: 		gsmnet->subscr_creation_mode ? "" : "no ", VTY_NEWLINE);
You change that to a switch in a follow-up commit? Then maybe have the enum only have the two values 0 / 1 so this doesn't look as fragile?
gerrit-no-reply@lists.osmocom.org May 20, 2016, 4:32 p.m. UTC | #9 
Patch Set 6: Code-Review+2
diff mbox

Patch

diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h
index f229e74..3c7f5d3 100644
--- a/openbsc/include/openbsc/gsm_data.h
+++ b/openbsc/include/openbsc/gsm_data.h
@@ -19,6 +19,12 @@ 
 
 #define OBSC_LINKID_CB(__msgb)	(__msgb)->cb[3]
 
+enum gsm_subscr_creation_mode {
+	GSM_SUBSCR_DONT_CREATE = 0,
+	GSM_SUBSCR_CREAT_W_RAND_EXT = 1,
+	GSM_SUBSCR_CREAT_W_NO_EXT = 2
+};
+
 enum gsm_security_event {
 	GSM_SECURITY_NOAVAIL,
 	GSM_SECURITY_AUTH_FAILED,
@@ -281,7 +287,7 @@ 
 	struct osmo_bsc_data *bsc_data;
 
 	/* subscriber related features */
-	int create_subscriber;
+	int subscr_creation_mode;
 	struct gsm_subscriber_group *subscr_group;
 	struct gsm_sms_queue *sms_queue;
 
diff --git a/openbsc/src/libbsc/net_init.c b/openbsc/src/libbsc/net_init.c
index 568a0b8..afcaaf3 100644
--- a/openbsc/src/libbsc/net_init.c
+++ b/openbsc/src/libbsc/net_init.c
@@ -48,7 +48,7 @@ 
 	INIT_LLIST_HEAD(&net->bsc_data->mscs);
 
 	net->subscr_group->net = net;
-	net->create_subscriber = 1;
+	net->subscr_creation_mode = GSM_SUBSCR_CREAT_W_RAND_EXT;
 
 	net->country_code = country_code;
 	net->network_code = network_code;
diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c
index f02f784..c93bcb5 100644
--- a/openbsc/src/libmsc/gsm_04_08.c
+++ b/openbsc/src/libmsc/gsm_04_08.c
@@ -507,6 +507,14 @@ 
 	return gsm48_conn_sendmsg(msg, conn, NULL);
 }
 
+static struct gsm_subscriber *subscr_create(const struct gsm_network *net,
+					    const char *imsi)
+{
+	if (GSM_SUBSCR_DONT_CREATE == net->subscr_creation_mode)
+		return NULL;
+
+	return subscr_create_subscriber(net->subscr_group, imsi);
+}
 
 /* Parse Chapter 9.2.11 Identity Response */
 static int mm_rx_id_resp(struct gsm_subscriber_connection *conn, struct msgb *msg)
@@ -530,9 +538,8 @@ 
 		if (!conn->subscr) {
 			conn->subscr = subscr_get_by_imsi(net->subscr_group,
 							  mi_string);
-			if (!conn->subscr && net->create_subscriber)
-				conn->subscr = subscr_create_subscriber(
-					net->subscr_group, mi_string);
+			if (!conn->subscr)
+				conn->subscr = subscr_create(net, mi_string);
 		}
 		if (!conn->subscr && conn->loc_operation) {
 			gsm0408_loc_upd_rej(conn, bts->network->reject_cause);
@@ -641,9 +648,8 @@ 
 
 		/* look up subscriber based on IMSI, create if not found */
 		subscr = subscr_get_by_imsi(bts->network->subscr_group, mi_string);
-		if (!subscr && bts->network->create_subscriber) {
-			subscr = subscr_create_subscriber(
-				bts->network->subscr_group, mi_string);
+		if (!subscr) {
+			subscr = subscr_create(	bts->network, mi_string);
 		}
 		if (!subscr) {
 			gsm0408_loc_upd_rej(conn, bts->network->reject_cause);
diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c
index 4c2088a..5d74e04 100644
--- a/openbsc/src/libmsc/vty_interface_layer3.c
+++ b/openbsc/src/libmsc/vty_interface_layer3.c
@@ -1036,7 +1036,7 @@ 
       "Make a new record when a subscriber is first seen.\n")
 {
 	struct gsm_network *gsmnet = gsmnet_from_vty(vty);
-	gsmnet->create_subscriber = 1;
+	gsmnet->subscr_creation_mode = GSM_SUBSCR_CREAT_W_RAND_EXT;
 	return CMD_SUCCESS;
 }
 
@@ -1045,7 +1045,7 @@ 
       NO_STR "Make a new record when a subscriber is first seen.\n")
 {
 	struct gsm_network *gsmnet = gsmnet_from_vty(vty);
-	gsmnet->create_subscriber = 0;
+	gsmnet->subscr_creation_mode = GSM_SUBSCR_DONT_CREATE;
 	return CMD_SUCCESS;
 }
 
@@ -1072,7 +1072,7 @@ 
 	struct gsm_network *gsmnet = gsmnet_from_vty(vty);
 	vty_out(vty, "nitb%s", VTY_NEWLINE);
 	vty_out(vty, " %ssubscriber-create-on-demand%s",
-		gsmnet->create_subscriber ? "" : "no ", VTY_NEWLINE);
+		gsmnet->subscr_creation_mode ? "" : "no ", VTY_NEWLINE);
 	vty_out(vty, " %sassign-tmsi%s",
 		gsmnet->avoid_tmsi ? "no " : "", VTY_NEWLINE);
 	return CMD_SUCCESS;