<div dir="ltr">Hi Holger,<div><br></div><div>Thank you for improving robustness of the code. I looked through it and it looks good to me.</div><div><br></div><div>Please let me know when you merge to the master so I could rebase the SMS validity decoding code on top of it and submit it for review.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Mar 8, 2014 at 1:54 PM, Holger Hans Peter Freyther <span dir="ltr"><<a href="mailto:holger@freyther.de" target="_blank">holger@freyther.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Holger Hans Peter Freyther <<a href="mailto:holger@moiji-mobile.com">holger@moiji-mobile.com</a>><br>
<br>
This is mostly based on Alexander's migration code. The code<br>
adds transaction handling and some sanity checks and cleanups<br>
to the code. We made the decision to fork the sms_from_result<br>
method and freeze it to that version. This way sms_from_result<br>
can move forward without having to deal with legacy.<br>
---<br>
openbsc/src/libbsc/gsm_subscriber_base.c | 6 +<br>
openbsc/src/libmsc/db.c | 209 ++++++++++++++++++++++++++++---<br>
2 files changed, 199 insertions(+), 16 deletions(-)<br>
<br>
diff --git a/openbsc/src/libbsc/gsm_subscriber_base.c b/openbsc/src/libbsc/gsm_subscriber_base.c<br>
index 5755687..5e00443 100644<br>
--- a/openbsc/src/libbsc/gsm_subscriber_base.c<br>
+++ b/openbsc/src/libbsc/gsm_subscriber_base.c<br>
@@ -72,6 +72,12 @@ static void subscr_free(struct gsm_subscriber *subscr)<br>
talloc_free(subscr);<br>
}<br>
<br>
+void subscr_direct_free(struct gsm_subscriber *subscr)<br>
+{<br>
+ OSMO_ASSERT(subscr->use_count == 1);<br>
+ subscr_free(subscr);<br>
+}<br>
+<br>
struct gsm_subscriber *subscr_get(struct gsm_subscriber *subscr)<br>
{<br>
subscr->use_count++;<br>
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c<br>
index 2351485..3b92691 100644<br>
--- a/openbsc/src/libmsc/db.c<br>
+++ b/openbsc/src/libmsc/db.c<br>
@@ -38,23 +38,42 @@<br>
#include <osmocom/core/statistics.h><br>
#include <osmocom/core/rate_ctr.h><br>
<br>
+/* Semi-Private-Interface (SPI) for the subscriber code */<br>
+void subscr_direct_free(struct gsm_subscriber *subscr);<br>
+<br>
static char *db_basename = NULL;<br>
static char *db_dirname = NULL;<br>
static dbi_conn conn;<br>
<br>
-#define SCHEMA_REVISION "3"<br>
+#define SCHEMA_REVISION "4"<br>
+<br>
+enum {<br>
+ SCHEMA_META,<br>
+ INSERT_META,<br>
+ SCHEMA_SUBSCRIBER,<br>
+ SCHEMA_AUTH,<br>
+ SCHEMA_EQUIPMENT,<br>
+ SCHEMA_EQUIPMENT_WATCH,<br>
+ SCHEMA_SMS,<br>
+ SCHEMA_VLR,<br>
+ SCHEMA_APDU,<br>
+ SCHEMA_COUNTERS,<br>
+ SCHEMA_RATE,<br>
+ SCHEMA_AUTHKEY,<br>
+ SCHEMA_AUTHLAST,<br>
+};<br>
<br>
-static char *create_stmts[] = {<br>
- "CREATE TABLE IF NOT EXISTS Meta ("<br>
+static const char *create_stmts[] = {<br>
+ [SCHEMA_META] = "CREATE TABLE IF NOT EXISTS Meta ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"key TEXT UNIQUE NOT NULL, "<br>
"value TEXT NOT NULL"<br>
")",<br>
- "INSERT OR IGNORE INTO Meta "<br>
+ [INSERT_META] = "INSERT OR IGNORE INTO Meta "<br>
"(key, value) "<br>
"VALUES "<br>
"('revision', " SCHEMA_REVISION ")",<br>
- "CREATE TABLE IF NOT EXISTS Subscriber ("<br>
+ [SCHEMA_SUBSCRIBER] = "CREATE TABLE IF NOT EXISTS Subscriber ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"created TIMESTAMP NOT NULL, "<br>
"updated TIMESTAMP NOT NULL, "<br>
@@ -66,13 +85,13 @@ static char *create_stmts[] = {<br>
"lac INTEGER NOT NULL DEFAULT 0, "<br>
"expire_lu TIMESTAMP DEFAULT NULL"<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS AuthToken ("<br>
+ [SCHEMA_AUTH] = "CREATE TABLE IF NOT EXISTS AuthToken ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"subscriber_id INTEGER UNIQUE NOT NULL, "<br>
"created TIMESTAMP NOT NULL, "<br>
"token TEXT UNIQUE NOT NULL"<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS Equipment ("<br>
+ [SCHEMA_EQUIPMENT] = "CREATE TABLE IF NOT EXISTS Equipment ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"created TIMESTAMP NOT NULL, "<br>
"updated TIMESTAMP NOT NULL, "<br>
@@ -82,7 +101,7 @@ static char *create_stmts[] = {<br>
"classmark3 BLOB, "<br>
"imei NUMERIC UNIQUE NOT NULL"<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS EquipmentWatch ("<br>
+ [SCHEMA_EQUIPMENT_WATCH] = "CREATE TABLE IF NOT EXISTS EquipmentWatch ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"created TIMESTAMP NOT NULL, "<br>
"updated TIMESTAMP NOT NULL, "<br>
@@ -90,7 +109,7 @@ static char *create_stmts[] = {<br>
"equipment_id NUMERIC NOT NULL, "<br>
"UNIQUE (subscriber_id, equipment_id) "<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS SMS ("<br>
+ [SCHEMA_SMS] = "CREATE TABLE IF NOT EXISTS SMS ("<br>
/* metadata, not part of sms */<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"created TIMESTAMP NOT NULL, "<br>
@@ -114,39 +133,39 @@ static char *create_stmts[] = {<br>
"header BLOB, " /* UD Header */<br>
"text TEXT " /* decoded UD after UDH */<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS VLR ("<br>
+ [SCHEMA_VLR] = "CREATE TABLE IF NOT EXISTS VLR ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"created TIMESTAMP NOT NULL, "<br>
"updated TIMESTAMP NOT NULL, "<br>
"subscriber_id NUMERIC UNIQUE NOT NULL, "<br>
"last_bts NUMERIC NOT NULL "<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS ApduBlobs ("<br>
+ [SCHEMA_APDU] = "CREATE TABLE IF NOT EXISTS ApduBlobs ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"created TIMESTAMP NOT NULL, "<br>
"apdu_id_flags INTEGER NOT NULL, "<br>
"subscriber_id INTEGER NOT NULL, "<br>
"apdu BLOB "<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS Counters ("<br>
+ [SCHEMA_COUNTERS] = "CREATE TABLE IF NOT EXISTS Counters ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"timestamp TIMESTAMP NOT NULL, "<br>
"value INTEGER NOT NULL, "<br>
"name TEXT NOT NULL "<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS RateCounters ("<br>
+ [SCHEMA_RATE] = "CREATE TABLE IF NOT EXISTS RateCounters ("<br>
"id INTEGER PRIMARY KEY AUTOINCREMENT, "<br>
"timestamp TIMESTAMP NOT NULL, "<br>
"value INTEGER NOT NULL, "<br>
"name TEXT NOT NULL, "<br>
"idx INTEGER NOT NULL "<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS AuthKeys ("<br>
+ [SCHEMA_AUTHKEY] = "CREATE TABLE IF NOT EXISTS AuthKeys ("<br>
"subscriber_id INTEGER PRIMARY KEY, "<br>
"algorithm_id INTEGER NOT NULL, "<br>
"a3a8_ki BLOB "<br>
")",<br>
- "CREATE TABLE IF NOT EXISTS AuthLastTuples ("<br>
+ [SCHEMA_AUTHLAST] = "CREATE TABLE IF NOT EXISTS AuthLastTuples ("<br>
"subscriber_id INTEGER PRIMARY KEY, "<br>
"issued TIMESTAMP NOT NULL, "<br>
"use_count INTEGER NOT NULL DEFAULT 0, "<br>
@@ -185,12 +204,164 @@ static int update_db_revision_2(void)<br>
"WHERE key = 'revision'");<br>
if (!result) {<br>
LOGP(DDB, LOGL_ERROR,<br>
- "Failed set new revision (upgrade vom rev 2).\n");<br>
+ "Failed to update DB schema revision (upgrade from rev 2).\n");<br>
+ return -EINVAL;<br>
+ }<br>
+ dbi_result_free(result);<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
+/**<br>
+ * Copied from the normal sms_from_result_v3 to avoid having<br>
+ * to make sure that the real routine will remain backward<br>
+ * compatible.<br>
+ */<br>
+static struct gsm_sms *sms_from_result_v3(dbi_result result)<br>
+{<br>
+ struct gsm_sms *sms = sms_alloc();<br>
+ long long unsigned int sender_id;<br>
+ struct gsm_subscriber *sender;<br>
+ const char *text, *daddr;<br>
+ const unsigned char *user_data;<br>
+ char buf[32];<br>
+<br>
+ if (!sms)<br>
+ return NULL;<br>
+<br>
+ sms->id = dbi_result_get_ulonglong(result, "id");<br>
+<br>
+ sender_id = dbi_result_get_ulonglong(result, "sender_id");<br>
+ snprintf(buf, sizeof(buf), "%llu", sender_id);<br>
+ sender = db_get_subscriber(GSM_SUBSCRIBER_ID, buf);<br>
+ OSMO_ASSERT(sender);<br>
+ strncpy(sms->src.addr, sender->extension, sizeof(sms->src.addr)-1);<br>
+ subscr_direct_free(sender);<br>
+ sender = NULL;<br>
+<br>
+ sms->reply_path_req = dbi_result_get_uint(result, "reply_path_req");<br>
+ sms->status_rep_req = dbi_result_get_uint(result, "status_rep_req");<br>
+ sms->ud_hdr_ind = dbi_result_get_uint(result, "ud_hdr_ind");<br>
+ sms->protocol_id = dbi_result_get_uint(result, "protocol_id");<br>
+ sms->data_coding_scheme = dbi_result_get_uint(result,<br>
+ "data_coding_scheme");<br>
+<br>
+ daddr = dbi_result_get_string(result, "dest_addr");<br>
+ if (daddr) {<br>
+ strncpy(sms->dst.addr, daddr, sizeof(sms->dst.addr));<br>
+ sms->dst.addr[sizeof(sms->dst.addr)-1] = '\0';<br>
+ }<br>
+<br>
+ sms->user_data_len = dbi_result_get_field_length(result, "user_data");<br>
+ user_data = dbi_result_get_binary(result, "user_data");<br>
+ if (sms->user_data_len > sizeof(sms->user_data))<br>
+ sms->user_data_len = (uint8_t) sizeof(sms->user_data);<br>
+ memcpy(sms->user_data, user_data, sms->user_data_len);<br>
+<br>
+ text = dbi_result_get_string(result, "text");<br>
+ if (text) {<br>
+ strncpy(sms->text, text, sizeof(sms->text));<br>
+ sms->text[sizeof(sms->text)-1] = '\0';<br>
+ }<br>
+ return sms;<br>
+}<br>
+<br>
+static int update_db_revision_3(void)<br>
+{<br>
+ dbi_result result;<br>
+ struct gsm_sms *sms;<br>
+<br>
+ result = dbi_conn_query(conn, "BEGIN EXCLUSIVE TRANSACTION");<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed to begin transaction (upgrade from rev 3)\n");<br>
return -EINVAL;<br>
}<br>
dbi_result_free(result);<br>
<br>
+ /* Rename old SMS table to be able create a new one */<br>
+ result = dbi_conn_query(conn, "ALTER TABLE SMS RENAME TO SMS_3");<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed to rename the old SMS table (upgrade from rev 3).\n");<br>
+ goto rollback;<br>
+ }<br>
+ dbi_result_free(result);<br>
+<br>
+ /* Create new SMS table with all the bells and whistles! */<br>
+ result = dbi_conn_query(conn, create_stmts[SCHEMA_SMS]);<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed to create a new SMS table (upgrade from rev 3).\n");<br>
+ goto rollback;<br>
+ }<br>
+ dbi_result_free(result);<br>
+<br>
+ /* Cycle through old messages and convert them to the new format */<br>
+ result = dbi_conn_queryf(conn, "SELECT * FROM SMS_3");<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed fetch messages from the old SMS table (upgrade from rev 3).\n");<br>
+ goto rollback;<br>
+ }<br>
+ while (dbi_result_next_row(result)) {<br>
+ sms = sms_from_result_v3(result);<br>
+ if (db_sms_store(sms) != 0) {<br>
+ LOGP(DDB, LOGL_ERROR, "Failed to store message to the new SMS table(upgrade from rev 3).\n");<br>
+ sms_free(sms);<br>
+ dbi_result_free(result);<br>
+ goto rollback;<br>
+ }<br>
+ sms_free(sms);<br>
+ }<br>
+ dbi_result_free(result);<br>
+<br>
+ /* Remove the temporary table */<br>
+ result = dbi_conn_query(conn, "DROP TABLE SMS_3");<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed to drop the old SMS table (upgrade from rev 3).\n");<br>
+ goto rollback;<br>
+ }<br>
+ dbi_result_free(result);<br>
+<br>
+ /* We're done. Bump DB Meta revision to 4 */<br>
+ result = dbi_conn_query(conn,<br>
+ "UPDATE Meta "<br>
+ "SET value = '4' "<br>
+ "WHERE key = 'revision'");<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed to update DB schema revision (upgrade from rev 3).\n");<br>
+ goto rollback;<br>
+ }<br>
+ dbi_result_free(result);<br>
+<br>
+ result = dbi_conn_query(conn, "COMMIT TRANSACTION");<br>
+ if (!result) {<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Failed to commit the transaction (upgrade from rev 3)\n");<br>
+ return -EINVAL;<br>
+ }<br>
+<br>
+ /* Shrink DB file size by actually wiping out SMS_3 table data */<br>
+ result = dbi_conn_query(conn, "VACUUM");<br>
+ if (!result)<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "VACUUM failed. Ignoring it (upgrade from rev 3).\n");<br>
+ else<br>
+ dbi_result_free(result);<br>
+<br>
return 0;<br>
+<br>
+rollback:<br>
+ result = dbi_conn_query(conn, "ROLLBACK TRANSACTION");<br>
+ if (!result)<br>
+ LOGP(DDB, LOGL_ERROR,<br>
+ "Rollback failed (upgrade from rev 3).\n");<br>
+ else<br>
+ dbi_result_free(result);<br>
+ return -EINVAL;<br>
}<br>
<br>
static int check_db_revision(void)<br>
@@ -218,6 +389,12 @@ static int check_db_revision(void)<br>
dbi_result_free(result);<br>
return -EINVAL;<br>
}<br>
+ } else if (!strcmp(rev_s, "3")) {<br>
+ if (update_db_revision_3()) {<br>
+ LOGP(DDB, LOGL_FATAL, "Failed to update database from schema revision '%s'.\n", rev_s);<br>
+ dbi_result_free(result);<br>
+ return -EINVAL;<br>
+ }<br>
} else if (!strcmp(rev_s, SCHEMA_REVISION)) {<br>
/* everything is fine */<br>
} else {<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.9.0<br>
<br>
<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Regards,<br>Alexander Chemeris.<br>CEO, Fairwaves, Inc. / ООО УмРадио<br><a href="https://fairwaves.co/" target="_blank">https://fairwaves.co</a><br>
</div>
</div>