[PATCH] osmo-hlr[master]: refactor db_subscr_lu()

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Wed Oct 11 20:32:29 UTC 2017


Hello Harald Welte, Jenkins Builder,

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

    https://gerrit.osmocom.org/4183

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

refactor db_subscr_lu()

Use named parameters in the SQL statement.
Use db_bind_* functions to drop some code dup.
Use explicit subscriber id arg instead of subscriber struct.
Match return values and error logging to other db functions.

Change-Id: I35665e84ddbe54a6f218b24033df969ad2e669a0
---
M src/db.c
M src/db.h
M src/db_hlr.c
M tests/db/db_test.c
M tests/db/db_test.err
5 files changed, 206 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/83/4183/3

diff --git a/src/db.c b/src/db.c
index 8286ba8..eb29434 100644
--- a/src/db.c
+++ b/src/db.c
@@ -45,8 +45,8 @@
 	[DB_STMT_SEL_BY_IMSI] = "SELECT " SEL_COLUMNS " FROM subscriber WHERE imsi = ?",
 	[DB_STMT_SEL_BY_MSISDN] = "SELECT " SEL_COLUMNS " FROM subscriber WHERE msisdn = ?",
 	[DB_STMT_SEL_BY_ID] = "SELECT " SEL_COLUMNS " FROM subscriber WHERE id = ?",
-	[DB_STMT_UPD_VLR_BY_ID] = "UPDATE subscriber SET vlr_number = ? WHERE id = ?",
-	[DB_STMT_UPD_SGSN_BY_ID] = "UPDATE subscriber SET sgsn_number = ? WHERE id = ?",
+	[DB_STMT_UPD_VLR_BY_ID] = "UPDATE subscriber SET vlr_number = $number WHERE id = $subscriber_id",
+	[DB_STMT_UPD_SGSN_BY_ID] = "UPDATE subscriber SET sgsn_number = $number WHERE id = $subscriber_id",
 	[DB_STMT_AUC_BY_IMSI] =
 		"SELECT id, algo_id_2g, ki, algo_id_3g, k, op, opc, sqn, ind_bitlen"
 		" FROM subscriber"
diff --git a/src/db.h b/src/db.h
index 5e234ff..9d4274d 100644
--- a/src/db.h
+++ b/src/db.h
@@ -91,10 +91,8 @@
 int db_subscr_get_by_id(struct db_context *dbc, int64_t id,
 			struct hlr_subscriber *subscr);
 int db_subscr_nam(struct db_context *dbc, const char *imsi, bool nam_val, bool is_ps);
-int db_subscr_lu(struct db_context *dbc,
-		 const struct hlr_subscriber *subscr,
-		 const char *vlr_or_sgsn_number,
-		 bool lu_is_ps);
+int db_subscr_lu(struct db_context *dbc, int64_t subscr_id,
+		 const char *vlr_or_sgsn_number, bool is_ps);
 
 int db_subscr_purge(struct db_context *dbc,
 		const char *imsi, bool is_ps);
diff --git a/src/db_hlr.c b/src/db_hlr.c
index 9bdb372..ac3c730 100644
--- a/src/db_hlr.c
+++ b/src/db_hlr.c
@@ -313,44 +313,46 @@
 	return ret;
 }
 
-int db_subscr_lu(struct db_context *dbc,
-		 const struct hlr_subscriber *subscr,
-		 const char *vlr_or_sgsn_number, bool lu_is_ps)
+int db_subscr_lu(struct db_context *dbc, int64_t subscr_id,
+		 const char *vlr_or_sgsn_number, bool is_ps)
 {
-	sqlite3_stmt *stmt = dbc->stmt[DB_STMT_UPD_VLR_BY_ID];
-	const char *txt;
+	sqlite3_stmt *stmt;
 	int rc, ret = 0;
 
-	if (lu_is_ps) {
-		stmt = dbc->stmt[DB_STMT_UPD_SGSN_BY_ID];
-		txt = subscr->sgsn_number;
-	} else {
-		stmt = dbc->stmt[DB_STMT_UPD_VLR_BY_ID];
-		txt = subscr->vlr_number;
-	}
+	stmt = dbc->stmt[is_ps ? DB_STMT_UPD_SGSN_BY_ID
+			       : DB_STMT_UPD_VLR_BY_ID];
 
-	rc = sqlite3_bind_int64(stmt, 1, subscr->id);
-	if (rc != SQLITE_OK) {
-		LOGP(DAUC, LOGL_ERROR, "Error binding ID: %d\n", rc);
-		return -EINVAL;
-	}
+	if (!db_bind_int64(stmt, "$subscriber_id", subscr_id))
+		return -EIO;
 
-	rc = sqlite3_bind_text(stmt, 2, txt, -1, SQLITE_STATIC);
-	if (rc != SQLITE_OK) {
-		LOGP(DAUC, LOGL_ERROR, "Error binding VLR/SGSN Number: %d\n", rc);
-		ret = -EBADMSG;
-		goto out;
-	}
+	if (!db_bind_text(stmt, "$number", vlr_or_sgsn_number))
+		return -EIO;
 
 	/* execute the statement */
 	rc = sqlite3_step(stmt);
 	if (rc != SQLITE_DONE) {
-		LOGP(DAUC, LOGL_ERROR, "Error updating SQN: %d\n", rc);
-		ret = -ENOEXEC;
+		LOGP(DAUC, LOGL_ERROR, "Update %s number for subscriber ID=%"PRId64": SQL Error: %s\n",
+		     is_ps? "SGSN" : "VLR", subscr_id, sqlite3_errmsg(dbc->db));
+		ret = -EIO;
+		goto out;
 	}
+
+	/* verify execution result */
+	rc = sqlite3_changes(dbc->db);
+	if (!rc) {
+		LOGP(DAUC, LOGL_ERROR, "Cannot update %s number for subscriber ID=%"PRId64
+		     ": no such subscriber\n",
+		     is_ps? "SGSN" : "VLR", subscr_id);
+		ret = -ENOENT;
+	} else if (rc != 1) {
+		LOGP(DAUC, LOGL_ERROR, "Update %s number for subscriber ID=%"PRId64
+		       ": SQL modified %d rows (expected 1)\n",
+		       is_ps? "SGSN" : "VLR", subscr_id, rc);
+		ret = -EIO;
+	}
+
 out:
 	db_remove_reset(stmt);
-
 	return ret;
 }
 
diff --git a/tests/db/db_test.c b/tests/db/db_test.c
index 30de51d..5acd1e8 100644
--- a/tests/db/db_test.c
+++ b/tests/db/db_test.c
@@ -283,6 +283,43 @@
 	ASSERT_RC(db_subscr_nam(dbc, "foobar", false, true), -ENOENT);
 	ASSERT_RC(db_subscr_nam(dbc, "foobar", false, false), -ENOENT);
 
+	comment("Record LU for PS and CS (SGSN and VLR names)");
+
+	ASSERT_RC(db_subscr_lu(dbc, id0, "5952", true), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, "712", false), 0);
+	ASSERT_SEL(id, id0, 0);
+
+	comment("Record LU for PS and CS (SGSN and VLR names) *again*");
+
+	ASSERT_RC(db_subscr_lu(dbc, id0, "111", true), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, "111", true), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, "222", false), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, "222", false), 0);
+	ASSERT_SEL(id, id0, 0);
+
+	comment("Unset LU info for PS and CS (SGSN and VLR names)");
+	ASSERT_RC(db_subscr_lu(dbc, id0, "", true), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, "", false), 0);
+	ASSERT_SEL(id, id0, 0);
+
+	ASSERT_RC(db_subscr_lu(dbc, id0, "111", true), 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, "222", false), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, NULL, true), 0);
+	ASSERT_SEL(id, id0, 0);
+	ASSERT_RC(db_subscr_lu(dbc, id0, NULL, false), 0);
+	ASSERT_SEL(id, id0, 0);
+
+	comment("Record LU for non-existent ID");
+	ASSERT_RC(db_subscr_lu(dbc, 99999, "5952", true), -ENOENT);
+	ASSERT_RC(db_subscr_lu(dbc, 99999, "712", false), -ENOENT);
+	ASSERT_SEL(id, 99999, -ENOENT);
+
 	comment("Delete non-existent / invalid IDs");
 
 	ASSERT_RC(db_subscr_delete_by_id(dbc, 999), -ENOENT);
diff --git a/tests/db/db_test.err b/tests/db/db_test.err
index f7c03d9..6e6a0ac 100644
--- a/tests/db/db_test.err
+++ b/tests/db/db_test.err
@@ -376,6 +376,143 @@
 DAUC Cannot disable CS: no such subscriber: IMSI='foobar'
 
 
+--- Record LU for PS and CS (SGSN and VLR names)
+
+db_subscr_lu(dbc, id0, "5952", true) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .sgsn_number = '5952',
+}
+
+db_subscr_lu(dbc, id0, "712", false) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '712',
+  .sgsn_number = '5952',
+}
+
+
+--- Record LU for PS and CS (SGSN and VLR names) *again*
+
+db_subscr_lu(dbc, id0, "111", true) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '712',
+  .sgsn_number = '111',
+}
+
+db_subscr_lu(dbc, id0, "111", true) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '712',
+  .sgsn_number = '111',
+}
+
+db_subscr_lu(dbc, id0, "222", false) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '222',
+  .sgsn_number = '111',
+}
+
+db_subscr_lu(dbc, id0, "222", false) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '222',
+  .sgsn_number = '111',
+}
+
+
+--- Unset LU info for PS and CS (SGSN and VLR names)
+
+db_subscr_lu(dbc, id0, "", true) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '222',
+}
+
+db_subscr_lu(dbc, id0, "", false) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+db_subscr_lu(dbc, id0, "111", true) --> 0
+
+db_subscr_lu(dbc, id0, "222", false) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '222',
+  .sgsn_number = '111',
+}
+
+db_subscr_lu(dbc, id0, NULL, true) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .vlr_number = '222',
+}
+
+db_subscr_lu(dbc, id0, NULL, false) --> 0
+
+db_subscr_get_by_id(dbc, id0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+
+--- Record LU for non-existent ID
+
+db_subscr_lu(dbc, 99999, "5952", true) --> -ENOENT
+DAUC Cannot update SGSN number for subscriber ID=99999: no such subscriber
+
+db_subscr_lu(dbc, 99999, "712", false) --> -ENOENT
+DAUC Cannot update VLR number for subscriber ID=99999: no such subscriber
+
+db_subscr_get_by_id(dbc, 99999, &g_subscr) --> -ENOENT
+DAUC Cannot read subscriber from db: ID=99999: No such subscriber
+
+
 --- Delete non-existent / invalid IDs
 
 db_subscr_delete_by_id(dbc, 999) --> -ENOENT

-- 
To view, visit https://gerrit.osmocom.org/4183
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35665e84ddbe54a6f218b24033df969ad2e669a0
Gerrit-PatchSet: 3
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder


More information about the gerrit-log mailing list