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.orgHello 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