[PATCH] osmo-hlr[master]: refactor db_subscr_purge

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

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/4184

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

refactor db_subscr_purge

Use named parameters in the SQL statements.

Use db_bind_* functions to drop some code dup.

Adopt error handling (rc and logging) to match the other db functions: return
-ENOENT for unknown subscriber, -EIO for SQL failures.

Change-Id: Iad49d29b90a708c6cf55bfb3bcc02d9e29001a15
---
M src/db.c
M src/db.h
M src/db_hlr.c
M src/hlr.c
M tests/db/db_test.c
M tests/db/db_test.err
6 files changed, 214 insertions(+), 17 deletions(-)


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

diff --git a/src/db.c b/src/db.c
index eb29434..2b2c2c4 100644
--- a/src/db.c
+++ b/src/db.c
@@ -54,8 +54,8 @@
 		" LEFT JOIN auc_3g ON auc_3g.subscriber_id = subscriber.id"
 		" WHERE imsi = ?",
 	[DB_STMT_AUC_UPD_SQN] = "UPDATE auc_3g SET sqn = ? WHERE subscriber_id = ?",
-	[DB_STMT_UPD_PURGE_CS_BY_IMSI] = "UPDATE subscriber SET ms_purged_cs=1 WHERE imsi = ?",
-	[DB_STMT_UPD_PURGE_PS_BY_IMSI] = "UPDATE subscriber SET ms_purged_ps=1 WHERE imsi = ?",
+	[DB_STMT_UPD_PURGE_CS_BY_IMSI] = "UPDATE subscriber SET ms_purged_cs = $val WHERE imsi = $imsi",
+	[DB_STMT_UPD_PURGE_PS_BY_IMSI] = "UPDATE subscriber SET ms_purged_ps = $val WHERE imsi = $imsi",
 	[DB_STMT_UPD_NAM_CS_BY_IMSI] = "UPDATE subscriber SET nam_cs = $val WHERE imsi = $imsi",
 	[DB_STMT_UPD_NAM_PS_BY_IMSI] = "UPDATE subscriber SET nam_ps = $val WHERE imsi = $imsi",
 	[DB_STMT_SUBSCR_CREATE] = "INSERT INTO subscriber (imsi) VALUES ($imsi)",
diff --git a/src/db.h b/src/db.h
index 9d4274d..2e6cc9b 100644
--- a/src/db.h
+++ b/src/db.h
@@ -94,5 +94,5 @@
 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);
+int db_subscr_purge(struct db_context *dbc, const char *by_imsi,
+		    bool purge_val, bool is_ps);
diff --git a/src/db_hlr.c b/src/db_hlr.c
index ac3c730..b6d224b 100644
--- a/src/db_hlr.c
+++ b/src/db_hlr.c
@@ -356,27 +356,49 @@
 	return ret;
 }
 
-int db_subscr_purge(struct db_context *dbc, const char *imsi, bool is_ps)
+int db_subscr_purge(struct db_context *dbc, const char *by_imsi,
+		    bool purge_val, bool is_ps)
 {
-	sqlite3_stmt *stmt = dbc->stmt[DB_STMT_UPD_VLR_BY_ID];
-	int rc, ret = 1;
+	sqlite3_stmt *stmt;
+	int rc, ret = 0;
 
-	if (is_ps)
-		stmt = dbc->stmt[DB_STMT_UPD_PURGE_PS_BY_IMSI];
-	else
-		stmt = dbc->stmt[DB_STMT_UPD_PURGE_CS_BY_IMSI];
+	stmt = dbc->stmt[is_ps ? DB_STMT_UPD_PURGE_PS_BY_IMSI
+			       : DB_STMT_UPD_PURGE_CS_BY_IMSI];
 
-	if (!db_bind_text(stmt, NULL, imsi))
-		return -EINVAL;
+	if (!db_bind_text(stmt, "$imsi", by_imsi))
+		return -EIO;
+	if (!db_bind_int(stmt, "$val", purge_val ? 1 : 0))
+		return -EIO;
 
 	/* execute the statement */
 	rc = sqlite3_step(stmt);
 	if (rc != SQLITE_DONE) {
-		LOGP(DAUC, LOGL_ERROR, "Error setting Purged: %d\n", rc);
-		ret = -ENOEXEC;
+		LOGP(DAUC, LOGL_ERROR, "%s %s: SQL error: %s\n",
+		     purge_val ? "purge" : "un-purge",
+		     is_ps ? "PS" : "CS",
+		     sqlite3_errmsg(dbc->db));
+		ret = -EIO;
+		goto out;
 	}
-	/* FIXME: return 0 in case IMSI not known */
 
+	/* verify execution result */
+	rc = sqlite3_changes(dbc->db);
+	if (!rc) {
+		LOGP(DAUC, LOGL_ERROR, "Cannot %s %s: no such subscriber: IMSI='%s'\n",
+		     purge_val ? "purge" : "un-purge",
+		     is_ps ? "PS" : "CS",
+		     by_imsi);
+		ret = -ENOENT;
+		goto out;
+	} else if (rc != 1) {
+		LOGHLR(by_imsi, LOGL_ERROR, "%s %s: SQL modified %d rows (expected 1)\n",
+		       purge_val ? "purge" : "un-purge",
+		       is_ps ? "PS" : "CS",
+		       rc);
+		ret = -EIO;
+	}
+
+out:
 	db_remove_reset(stmt);
 
 	return ret;
diff --git a/src/hlr.c b/src/hlr.c
index 9e8b699..b32f709 100644
--- a/src/hlr.c
+++ b/src/hlr.c
@@ -221,7 +221,7 @@
 	 * we have on record. Only update if yes */
 
 	/* Perform the actual update of the DB */
-	rc = db_subscr_purge(g_hlr->dbc, gsup->imsi, is_ps);
+	rc = db_subscr_purge(g_hlr->dbc, gsup->imsi, true, is_ps);
 
 	if (rc == 1)
 		gsup_reply.message_type = OSMO_GSUP_MSGT_PURGE_MS_RESULT;
diff --git a/tests/db/db_test.c b/tests/db/db_test.c
index 5acd1e8..20553ef 100644
--- a/tests/db/db_test.c
+++ b/tests/db/db_test.c
@@ -320,6 +320,44 @@
 	ASSERT_RC(db_subscr_lu(dbc, 99999, "712", false), -ENOENT);
 	ASSERT_SEL(id, 99999, -ENOENT);
 
+	comment("Purge and un-purge PS and CS");
+
+	/*                               purge_val, is_ps */
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, true, true), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, true, false), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, false, false), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, false, true), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+
+	comment("Purge PS and CS *again*");
+
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, true, true), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, true, true), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, false, true), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, false, true), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, true, false), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, true, false), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, false, false), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+	ASSERT_RC(db_subscr_purge(dbc, imsi0, false, false), 0);
+	ASSERT_SEL(imsi, imsi0, 0);
+
+	comment("Purge on non-existent / invalid IMSI");
+
+	ASSERT_RC(db_subscr_purge(dbc, unknown_imsi, true, true), -ENOENT);
+	ASSERT_SEL(imsi, unknown_imsi, -ENOENT);
+	ASSERT_RC(db_subscr_purge(dbc, unknown_imsi, true, false), -ENOENT);
+	ASSERT_SEL(imsi, unknown_imsi, -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 6e6a0ac..59b9ba1 100644
--- a/tests/db/db_test.err
+++ b/tests/db/db_test.err
@@ -513,6 +513,143 @@
 DAUC Cannot read subscriber from db: ID=99999: No such subscriber
 
 
+--- Purge and un-purge PS and CS
+
+db_subscr_purge(dbc, imsi0, true, true) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_ps = true,
+}
+
+db_subscr_purge(dbc, imsi0, true, false) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_cs = true,
+  .ms_purged_ps = true,
+}
+
+db_subscr_purge(dbc, imsi0, false, false) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_ps = true,
+}
+
+db_subscr_purge(dbc, imsi0, false, true) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+
+--- Purge PS and CS *again*
+
+db_subscr_purge(dbc, imsi0, true, true) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_ps = true,
+}
+
+db_subscr_purge(dbc, imsi0, true, true) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_ps = true,
+}
+
+db_subscr_purge(dbc, imsi0, false, true) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+db_subscr_purge(dbc, imsi0, false, true) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+db_subscr_purge(dbc, imsi0, true, false) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_cs = true,
+}
+
+db_subscr_purge(dbc, imsi0, true, false) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+  .ms_purged_cs = true,
+}
+
+db_subscr_purge(dbc, imsi0, false, false) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+db_subscr_purge(dbc, imsi0, false, false) --> 0
+
+db_subscr_get_by_imsi(dbc, imsi0, &g_subscr) --> 0
+struct hlr_subscriber {
+  .id = 1,
+  .imsi = '123456789000000',
+  .msisdn = '543210123456789',
+}
+
+
+--- Purge on non-existent / invalid IMSI
+
+db_subscr_purge(dbc, unknown_imsi, true, true) --> -ENOENT
+DAUC Cannot purge PS: no such subscriber: IMSI='999999999'
+
+db_subscr_get_by_imsi(dbc, unknown_imsi, &g_subscr) --> -ENOENT
+DAUC Cannot read subscriber from db: IMSI='999999999': No such subscriber
+
+db_subscr_purge(dbc, unknown_imsi, true, false) --> -ENOENT
+DAUC Cannot purge CS: no such subscriber: IMSI='999999999'
+
+db_subscr_get_by_imsi(dbc, unknown_imsi, &g_subscr) --> -ENOENT
+DAUC Cannot read subscriber from db: IMSI='999999999': No such subscriber
+
+
 --- Delete non-existent / invalid IDs
 
 db_subscr_delete_by_id(dbc, 999) --> -ENOENT

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad49d29b90a708c6cf55bfb3bcc02d9e29001a15
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