diff mbox

Change in openbsc[master]: move to hex TMSI representation

Message ID gerrit.1463134643373.I518c441f11f234b8f34ede5b76671049f06b6599@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 13, 2016, 10:17 a.m. UTC 
From Vadim Yanitskiy <axilirator@gmail.com>:

Vadim Yanitskiy has uploaded a new change for review.

  https://gerrit.osmocom.org/63

Change subject: move to hex TMSI representation
......................................................................

move to hex TMSI representation

In OpenBSC, we traditionally displayed a TMSI in its integer
representation, which is quite unusual in the telecom world.
A TMSI is normally printed as a series of 8 hex digits.

This patch aligns OpenBSC with the telecom industry standard
and should be applied with corresponding patch for libosmocore.

- Use hex representation in VTY
- Increased DB SCHEMA_REVISION
- Implemented DB migration code

Change-Id: I518c441f11f234b8f34ede5b76671049f06b6599
---
M openbsc/include/openbsc/gsm_subscriber.h
M openbsc/src/libmsc/db.c
M openbsc/src/libmsc/vty_interface_layer3.c
M openbsc/tests/db/db_test.c
M openbsc/tests/db/db_test.err
M openbsc/tests/gsm0408/gsm0408_test.c
6 files changed, 138 insertions(+), 24 deletions(-)


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

Comments

gerrit-no-reply@lists.osmocom.org May 13, 2016, 10:54 a.m. UTC | #1 
From Vadim Yanitskiy <axilirator@gmail.com>:

Vadim Yanitskiy has posted comments on this change.

Change subject: move to hex TMSI representation
......................................................................


Patch Set 1:

> Build Failed
 > 
 > http://jenkins.osmocom.org/jenkins/job/OpenBSC-gerrit/12/ : FAILURE

This code will work only after applying a previous patch.
On my host 'make check' works without any problems.
gerrit-no-reply@lists.osmocom.org May 20, 2016, 7:16 p.m. UTC | #2 
Patch Set 2:

(5 comments)

Argh. I just noticed an issue with the libosmocore change. I will need to revert it. We want to be able to use old OpenBSC code against newer libosmocore and this can only be done... if int->string->int code is in libosmocore. :(

https://gerrit.osmocom.org/#/c/63/2/openbsc/include/openbsc/gsm_subscriber.h
File openbsc/include/openbsc/gsm_subscriber.h:

PS2, Line 16: define
We need to move this to libosmocore. I just noticed that by accepting the change in libosmocore but not this one. We actually broke the parsing of TMSIs and LUs and CM Service Request.. 

We need to have both in libosmocore.


https://gerrit.osmocom.org/#/c/63/2/openbsc/src/libmsc/db.c
File openbsc/src/libmsc/db.c:

Line 216: 	LOGP(DDB, LOGL_NOTICE, "Migration complete.\n");
Why this hunk?


Line 237: 	sms->id   = dbi_result_get_ulonglong(result, "id");
Why that? I mean v3 is old. We should not see this right now?


Line 376: 	return -EINVAL;
Please put that in a separate commit


Line 417: 		"INSERT INTO Subscriber "
You rely on sqlite doing type conversion from string to number here?
gerrit-no-reply@lists.osmocom.org May 20, 2016, 7:16 p.m. UTC | #3 
Patch Set 2: Code-Review-1
diff mbox

Patch

diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h
index 9df989a..7e9a272 100644
--- a/openbsc/include/openbsc/gsm_subscriber.h
+++ b/openbsc/include/openbsc/gsm_subscriber.h
@@ -13,7 +13,7 @@ 
 
 #define GSM_SUBSCRIBER_FIRST_CONTACT	0x00000001
 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */
-#define tmsi_from_string(str) strtoul(str, NULL, 10)
+#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
 
 #define GSM_SUBSCRIBER_NO_EXPIRATION	0x0
 
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c
index b3235bb..087e2cd 100644
--- a/openbsc/src/libmsc/db.c
+++ b/openbsc/src/libmsc/db.c
@@ -48,7 +48,7 @@ 
 static char *db_dirname = NULL;
 static dbi_conn conn;
 
-#define SCHEMA_REVISION "4"
+#define SCHEMA_REVISION "5"
 
 enum {
 	SCHEMA_META,
@@ -84,7 +84,7 @@ 
 		"name TEXT, "
 		"extension TEXT UNIQUE, "
 		"authorized INTEGER NOT NULL DEFAULT 0, "
-		"tmsi TEXT UNIQUE, "
+		"tmsi INTEGER UNIQUE, "
 		"lac INTEGER NOT NULL DEFAULT 0, "
 		"expire_lu TIMESTAMP DEFAULT NULL"
 		")",
@@ -213,6 +213,7 @@ 
 	}
 	dbi_result_free(result);
 
+	LOGP(DDB, LOGL_NOTICE, "Migration complete.\n");
 	return 0;
 }
 
@@ -225,23 +226,27 @@ 
 {
 	struct gsm_sms *sms = sms_alloc();
 	long long unsigned int sender_id;
-	struct gsm_subscriber *sender;
-	const char *text, *daddr;
+	const char *text, *daddr, *extension;
 	const unsigned char *user_data;
-	char buf[32];
+	dbi_result sender_result;
 
 	if (!sms)
 		return NULL;
 
-	sms->id = dbi_result_get_ulonglong(result, "id");
-
 	sender_id = dbi_result_get_ulonglong(result, "sender_id");
-	snprintf(buf, sizeof(buf), "%llu", sender_id);
-	sender = db_get_subscriber(GSM_SUBSCRIBER_ID, buf);
-	OSMO_ASSERT(sender);
-	strncpy(sms->src.addr, sender->extension, sizeof(sms->src.addr)-1);
-	subscr_direct_free(sender);
-	sender = NULL;
+	sms->id   = dbi_result_get_ulonglong(result, "id");
+
+	sender_result = dbi_conn_queryf(conn,
+		"SELECT * FROM Subscriber "
+		"WHERE id = %llu", sender_id);
+
+	if (sender_result) {
+		if (dbi_result_next_row(sender_result)) {
+			extension = dbi_result_get_string(sender_result, "extension");
+			strncpy(sms->src.addr, extension, sizeof(sms->src.addr) - 1);
+		}
+		dbi_result_free(sender_result);
+	}
 
 	sms->reply_path_req = dbi_result_get_ulonglong(result, "reply_path_req");
 	sms->status_rep_req = dbi_result_get_ulonglong(result, "status_rep_req");
@@ -358,6 +363,7 @@ 
 	else
 		dbi_result_free(result);
 
+	LOGP(DDB, LOGL_NOTICE, "Migration complete.\n");
 	return 0;
 
 rollback:
@@ -365,6 +371,108 @@ 
 	if (!result)
 		LOGP(DDB, LOGL_ERROR,
 			"Rollback failed (upgrade from rev 3).\n");
+	else
+		dbi_result_free(result);
+	return -EINVAL;
+}
+
+static int update_db_revision_4(void)
+{
+	dbi_result result;
+
+	LOGP(DDB, LOGL_NOTICE, "Going to migrate from revision 4\n");
+
+	result = dbi_conn_query(conn, "BEGIN EXCLUSIVE TRANSACTION");
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+			"Failed to begin transaction "
+			"(upgrade from rev 4)\n");
+		return -EINVAL;
+	}
+	dbi_result_free(result);
+
+	/* Rename old Subscriber table to be able create a new one */
+	result = dbi_conn_query(conn,
+		"ALTER TABLE Subscriber RENAME TO Subscriber_4");
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+		     "Failed to rename the old Subscriber table "
+		     "(upgrade from rev 4).\n");
+		goto rollback;
+	}
+	dbi_result_free(result);
+
+	/* Create new Subscriber table */
+	result = dbi_conn_query(conn, create_stmts[SCHEMA_SUBSCRIBER]);
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+		     "Failed to create a new Subscriber table "
+		     "(upgrade from rev 4).\n");
+		goto rollback;
+	}
+	dbi_result_free(result);
+
+	/* Copy subscriber data into the new table */
+	result = dbi_conn_query(conn,
+		"INSERT INTO Subscriber "
+		"SELECT * FROM Subscriber_4");
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+		     "Failed to copy subscriber data into the new table "
+		     "(upgrade from rev 4).\n");
+		goto rollback;
+	}
+	dbi_result_free(result);
+
+	/* Remove the temporary table */
+	result = dbi_conn_query(conn, "DROP TABLE Subscriber_4");
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+		     "Failed to drop the old Subscriber table "
+		     "(upgrade from rev 4).\n");
+		goto rollback;
+	}
+	dbi_result_free(result);
+
+	/* We're done. Bump DB Meta revision to 5 */
+	result = dbi_conn_query(conn,
+				"UPDATE Meta "
+				"SET value = '5' "
+				"WHERE key = 'revision'");
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+		     "Failed to update DB schema revision "
+		     "(upgrade from rev 4).\n");
+		goto rollback;
+	}
+	dbi_result_free(result);
+
+	result = dbi_conn_query(conn, "COMMIT TRANSACTION");
+	if (!result) {
+		LOGP(DDB, LOGL_ERROR,
+			"Failed to commit the transaction "
+			"(upgrade from rev 4)\n");
+		return -EINVAL;
+	}
+
+	/* Shrink DB file size by actually wiping out Subscriber_4 table data */
+	result = dbi_conn_query(conn, "VACUUM");
+	if (!result)
+		LOGP(DDB, LOGL_ERROR,
+			"VACUUM failed. Ignoring it "
+			"(upgrade from rev 4).\n");
+	else
+		dbi_result_free(result);
+
+	LOGP(DDB, LOGL_NOTICE, "Migration complete.\n");
+	return 0;
+
+rollback:
+	result = dbi_conn_query(conn, "ROLLBACK TRANSACTION");
+	if (!result)
+		LOGP(DDB, LOGL_ERROR,
+			"Rollback failed "
+			"(upgrade from rev 4).\n");
 	else
 		dbi_result_free(result);
 	return -EINVAL;
@@ -412,6 +520,9 @@ 
 			goto error;
 	case 3:
 		if (update_db_revision_3())
+			goto error;
+	case 4:
+		if (update_db_revision_4())
 			goto error;
 
 	/* The end of waterfall */
@@ -823,10 +934,6 @@ 
 	if (string)
 		strncpy(subscr->imsi, string, sizeof(subscr->imsi)-1);
 
-	string = dbi_result_get_string(result, "tmsi");
-	if (string)
-		subscr->tmsi = tmsi_from_string(string);
-
 	string = dbi_result_get_string(result, "name");
 	if (string)
 		strncpy(subscr->name, string, GSM_NAME_LENGTH);
@@ -836,6 +943,7 @@ 
 		strncpy(subscr->extension, string, GSM_EXTENSION_LENGTH);
 
 	subscr->lac = dbi_result_get_ulonglong(result, "lac");
+	subscr->tmsi = dbi_result_get_ulonglong(result, "tmsi");
 
 	if (!dbi_result_field_is_null(result, "expire_lu"))
 		subscr->expire_lu = dbi_result_get_datetime(result, "expire_lu");
@@ -908,9 +1016,10 @@ 
 	subscr->id = dbi_result_get_ulonglong(result, "id");
 
 	db_set_from_query(subscr, result);
-	DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', TMSI %u, EXTEN '%s', LAC %hu, AUTH %u\n",
-		subscr->id, subscr->imsi, subscr->name, subscr->tmsi, subscr->extension,
-		subscr->lac, subscr->authorized);
+	DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', "
+		"TMSI 0x%08x, EXTEN '%s', LAC %hu, AUTH %u\n",
+		subscr->id, subscr->imsi, subscr->name, subscr->tmsi,
+		subscr->extension, subscr->lac, subscr->authorized);
 	dbi_result_free(result);
 
 	get_equipment_by_subscr(subscr);
@@ -1240,7 +1349,7 @@ 
 		}
 		if (!dbi_result_next_row(result)) {
 			dbi_result_free(result);
-			DEBUGP(DDB, "Allocated TMSI %u for IMSI %s.\n",
+			DEBUGP(DDB, "Allocated TMSI 0x%08x for IMSI %s.\n",
 				subscriber->tmsi, subscriber->imsi);
 			return db_sync_subscriber(subscriber);
 		}
diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c
index 4c2088a..2fa04c0 100644
--- a/openbsc/src/libmsc/vty_interface_layer3.c
+++ b/openbsc/src/libmsc/vty_interface_layer3.c
@@ -189,7 +189,8 @@ 
 	else if (!strcmp(type, "imsi"))
 		return subscr_get_by_imsi(gsmnet->subscr_group, id);
 	else if (!strcmp(type, "tmsi"))
-		return subscr_get_by_tmsi(gsmnet->subscr_group, atoi(id));
+		return subscr_get_by_tmsi(gsmnet->subscr_group,
+			tmsi_from_string(id));
 	else if (!strcmp(type, "id"))
 		return subscr_get_by_id(gsmnet->subscr_group, atoi(id));
 
diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c
index fb159a5..4aa537c 100644
--- a/openbsc/tests/db/db_test.c
+++ b/openbsc/tests/db/db_test.c
@@ -187,6 +187,7 @@ 
 
 	char *alice_imsi = "3243245432345";
 	alice = db_create_subscriber(alice_imsi);
+	db_subscriber_alloc_tmsi(alice);
 	db_sync_subscriber(alice);
 	alice_db = db_get_subscriber(GSM_SUBSCRIBER_IMSI, alice->imsi);
 	COMPARE(alice, alice_db);
diff --git a/openbsc/tests/db/db_test.err b/openbsc/tests/db/db_test.err
index fa9a54c..d8a3e7f 100644
--- a/openbsc/tests/db/db_test.err
+++ b/openbsc/tests/db/db_test.err
@@ -1,2 +1,5 @@ 
 Going to migrate from revision 3
+Migration complete.
+Going to migrate from revision 4
+Migration complete.
 
\ No newline at end of file
diff --git a/openbsc/tests/gsm0408/gsm0408_test.c b/openbsc/tests/gsm0408/gsm0408_test.c
index 9262667..6b1b7ce 100644
--- a/openbsc/tests/gsm0408/gsm0408_test.c
+++ b/openbsc/tests/gsm0408/gsm0408_test.c
@@ -195,7 +195,7 @@ 
 	/* tmsi code */
 	mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi);
 	gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2);
-	COMPARE((uint32_t)strtoul(mi_parsed, NULL, 10), ==, tmsi);
+	COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi);
 
 	/* imsi code */
 	mi_len = gsm48_generate_mid_from_imsi(mi, imsi_odd);