This patchset solves the issue with the wrong sender address for inbound SMPP SMS messages. It then further cleans up DB structure to remove 'receiver_id' field from the sms table.
The only issue with this code is that it somehow breaks channel_test test, because it depended on some fragile linker behavior. The last patch in the series fixes compilation of the test, but the test asserts. I hope the test author will find a more proper way to implement it, but at this moment I propose to disable it.
Alexander Chemeris (4): bsc: Allow subscr_put() to be called with subscr->net=NULL. sms: Add a function to update DB scheme v3 to v4. msc: Do not store received id in the SMS database. This is a hack to fix channel_test.c compilation.
Holger Hans Peter Freyther (1): sms: Kill the sms->sender and use addr/ton/npi throughout the code
openbsc/include/openbsc/gsm_data.h | 1 - openbsc/src/libbsc/gsm_subscriber_base.c | 2 +- openbsc/src/libmsc/db.c | 257 ++++++++++++++++++++++++------ openbsc/src/libmsc/gsm_04_11.c | 9 +- openbsc/src/libmsc/smpp_openbsc.c | 5 +- openbsc/tests/channel/Makefile.am | 4 +- openbsc/tests/channel/channel_test.c | 12 -- 7 files changed, 215 insertions(+), 75 deletions(-)
This is useful for tests and is required for the v3 -> v4 DB convertion code. --- openbsc/src/libbsc/gsm_subscriber_base.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/openbsc/src/libbsc/gsm_subscriber_base.c b/openbsc/src/libbsc/gsm_subscriber_base.c index 747a699..9ea0584 100644 --- a/openbsc/src/libbsc/gsm_subscriber_base.c +++ b/openbsc/src/libbsc/gsm_subscriber_base.c @@ -85,7 +85,7 @@ struct gsm_subscriber *subscr_put(struct gsm_subscriber *subscr) subscr->use_count--; DEBUGP(DREF, "subscr %s usage decreased usage to: %d\n", subscr->extension, subscr->use_count); - if (subscr->use_count <= 0 && !subscr->net->keep_subscr) + if (subscr->use_count <= 0 && (!subscr->net || !subscr->net->keep_subscr)) subscr_free(subscr); return NULL; }
On Tue, Oct 08, 2013 at 03:17:29AM +0200, Alexander Chemeris wrote:
- if (subscr->use_count <= 0 && !subscr->net->keep_subscr)
- if (subscr->use_count <= 0 && (!subscr->net || !subscr->net->keep_subscr))
okay, that is a difficult one. I posted my first patch to remove the network from the db code. So maybe we expose subscr_free and the db code (and test code) will just directly free the subscriber (or abort when the use_count != 1)?
What do you think?
From: Holger Hans Peter Freyther zecke@selfish.org
This is an incompatible database schema change. Store the type of the address in the database for both the sender and the receiver.
Currently it is possible to use SMPP to store a SMS and the NPI and TON will be lost on the delivery of the SMS. The schema is changed to make the delivery always use the right NPI/TON. This patch is not ready for the master branch as there is no upgrade path for the HLR yet. --- openbsc/include/openbsc/gsm_data.h | 1 - openbsc/src/libmsc/db.c | 56 ++++++++++++++++++++++++------------ openbsc/src/libmsc/gsm_04_11.c | 9 ++---- openbsc/src/libmsc/smpp_openbsc.c | 5 ++-- 4 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h index 8741505..45b5784 100644 --- a/openbsc/include/openbsc/gsm_data.h +++ b/openbsc/include/openbsc/gsm_data.h @@ -302,7 +302,6 @@ struct gsm_sms_addr {
struct gsm_sms { unsigned long long id; - struct gsm_subscriber *sender; struct gsm_subscriber *receiver; struct gsm_sms_addr src, dst; enum gsm_sms_source_id source; diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 9a8cd88..8f76324 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -42,7 +42,7 @@ static char *db_basename = NULL; static char *db_dirname = NULL; static dbi_conn conn;
-#define SCHEMA_REVISION "3" +#define SCHEMA_REVISION "50000000"
static char *create_stmts[] = { "CREATE TABLE IF NOT EXISTS Meta (" @@ -95,7 +95,6 @@ static char *create_stmts[] = { "id INTEGER PRIMARY KEY AUTOINCREMENT, " "created TIMESTAMP NOT NULL, " "sent TIMESTAMP, " - "sender_id INTEGER NOT NULL, " "receiver_id INTEGER NOT NULL, " "deliver_attempts INTEGER NOT NULL DEFAULT 0, " /* data directly copied/derived from SMS */ @@ -105,7 +104,12 @@ static char *create_stmts[] = { "protocol_id INTEGER NOT NULL, " "data_coding_scheme INTEGER NOT NULL, " "ud_hdr_ind INTEGER NOT NULL, " - "dest_addr TEXT, " + "src_addr TEXT NOT NULL, " + "src_ton INTEGER NOT NULL, " + "src_npi INTEGER NOT NULL, " + "dest_addr TEXT NOT NULL, " + "dest_ton INTEGER NOT NULL, " + "dest_npi INTEGER NOT NULL, " "user_data BLOB, " /* TP-UD */ /* additional data, interpreted from SMS */ "header BLOB, " /* UD Header */ @@ -1096,7 +1100,7 @@ int db_subscriber_assoc_imei(struct gsm_subscriber *subscriber, char imei[GSM_IM int db_sms_store(struct gsm_sms *sms) { dbi_result result; - char *q_text, *q_daddr; + char *q_text, *q_daddr, *q_saddr; unsigned char *q_udata; char *validity_timestamp = "2222-2-2";
@@ -1104,25 +1108,35 @@ int db_sms_store(struct gsm_sms *sms)
dbi_conn_quote_string_copy(conn, (char *)sms->text, &q_text); dbi_conn_quote_string_copy(conn, (char *)sms->dst.addr, &q_daddr); + dbi_conn_quote_string_copy(conn, (char *)sms->src.addr, &q_saddr); dbi_conn_quote_binary_copy(conn, sms->user_data, sms->user_data_len, &q_udata); + /* FIXME: correct validity period */ result = dbi_conn_queryf(conn, "INSERT INTO SMS " - "(created, sender_id, receiver_id, valid_until, " + "(created, receiver_id, valid_until, " "reply_path_req, status_rep_req, protocol_id, " - "data_coding_scheme, ud_hdr_ind, dest_addr, " - "user_data, text) VALUES " - "(datetime('now'), %llu, %llu, %u, " - "%u, %u, %u, %u, %u, %s, %s, %s)", - sms->sender->id, + "data_coding_scheme, ud_hdr_ind, " + "user_data, text, " + "dest_addr, dest_ton, dest_npi, " + "src_addr, src_ton, src_npi) VALUES " + "(datetime('now'), %llu, %u, " + "%u, %u, %u, " + "%u, %u, " + "%s, %s, " + "%s, %u, %u, " + "%s, %u, %u)", sms->receiver ? sms->receiver->id : 0, validity_timestamp, sms->reply_path_req, sms->status_rep_req, sms->protocol_id, sms->data_coding_scheme, sms->ud_hdr_ind, - q_daddr, q_udata, q_text); + q_udata, q_text, + q_daddr, sms->dst.ton, sms->dst.npi, + q_saddr, sms->src.ton, sms->src.npi); free(q_text); - free(q_daddr); free(q_udata); + free(q_daddr); + free(q_saddr);
if (!result) return -EIO; @@ -1134,8 +1148,8 @@ int db_sms_store(struct gsm_sms *sms) static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result result) { struct gsm_sms *sms = sms_alloc(); - long long unsigned int sender_id, receiver_id; - const char *text, *daddr; + long long unsigned int receiver_id; + const char *text, *daddr, *saddr; const unsigned char *user_data;
if (!sms) @@ -1143,10 +1157,6 @@ static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result resul
sms->id = dbi_result_get_ulonglong(result, "id");
- sender_id = dbi_result_get_ulonglong(result, "sender_id"); - sms->sender = subscr_get_by_id(net, sender_id); - strncpy(sms->src.addr, sms->sender->extension, sizeof(sms->src.addr)-1); - receiver_id = dbi_result_get_ulonglong(result, "receiver_id"); sms->receiver = subscr_get_by_id(net, receiver_id);
@@ -1160,12 +1170,22 @@ static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result resul "data_coding_scheme"); /* sms->msg_ref is temporary and not stored in DB */
+ sms->dst.npi = dbi_result_get_uint(result, "dest_npi"); + sms->dst.ton = dbi_result_get_uint(result, "dest_ton"); daddr = dbi_result_get_string(result, "dest_addr"); if (daddr) { strncpy(sms->dst.addr, daddr, sizeof(sms->dst.addr)); sms->dst.addr[sizeof(sms->dst.addr)-1] = '\0'; }
+ sms->src.npi = dbi_result_get_uint(result, "src_npi"); + sms->src.ton = dbi_result_get_uint(result, "src_ton"); + saddr = dbi_result_get_string(result, "src_addr"); + if (saddr) { + strncpy(sms->src.addr, saddr, sizeof(sms->src.addr)); + sms->src.addr[sizeof(sms->src.addr)-1] = '\0'; + } + sms->user_data_len = dbi_result_get_field_length(result, "user_data"); user_data = dbi_result_get_binary(result, "user_data"); if (sms->user_data_len > sizeof(sms->user_data)) diff --git a/openbsc/src/libmsc/gsm_04_11.c b/openbsc/src/libmsc/gsm_04_11.c index e554b74..96da04b 100644 --- a/openbsc/src/libmsc/gsm_04_11.c +++ b/openbsc/src/libmsc/gsm_04_11.c @@ -73,8 +73,6 @@ struct gsm_sms *sms_alloc(void) void sms_free(struct gsm_sms *sms) { /* drop references to subscriber structure */ - if (sms->sender) - subscr_put(sms->sender); if (sms->receiver) subscr_put(sms->receiver); #ifdef BUILD_SMPP @@ -97,8 +95,7 @@ struct gsm_sms *sms_from_text(struct gsm_subscriber *receiver, sms->receiver = subscr_get(receiver); strncpy(sms->text, text, sizeof(sms->text)-1);
- sms->sender = subscr_get(sender); - strncpy(sms->src.addr, sms->sender->extension, sizeof(sms->src.addr)-1); + strncpy(sms->src.addr, sender->extension, sizeof(sms->src.addr)-1); sms->reply_path_req = 0; sms->status_rep_req = 0; sms->ud_hdr_ind = 0; @@ -377,12 +374,10 @@ static int gsm340_rx_tpdu(struct gsm_subscriber_connection *conn, struct msgb *m } }
- gsms->sender = subscr_get(conn->subscr); - LOGP(DLSMS, LOGL_INFO, "RX SMS: Sender: %s, MTI: 0x%02x, VPF: 0x%02x, " "MR: 0x%02x PID: 0x%02x, DCS: 0x%02x, DA: %s, " "UserDataLength: 0x%02x, UserData: "%s"\n", - subscr_name(gsms->sender), sms_mti, sms_vpf, gsms->msg_ref, + subscr_name(conn->subscr), sms_mti, sms_vpf, gsms->msg_ref, gsms->protocol_id, gsms->data_coding_scheme, gsms->dst.addr, gsms->user_data_len, sms_alphabet == DCS_7BIT_DEFAULT ? gsms->text : diff --git a/openbsc/src/libmsc/smpp_openbsc.c b/openbsc/src/libmsc/smpp_openbsc.c index 9d2183a..078fecc 100644 --- a/openbsc/src/libmsc/smpp_openbsc.c +++ b/openbsc/src/libmsc/smpp_openbsc.c @@ -131,7 +131,6 @@ static int submit_to_sms(struct gsm_sms **psms, struct gsm_network *net, strncpy(sms->dst.addr, dest->extension, sizeof(sms->dst.addr)-1);
/* fill in the source address */ - sms->sender = subscr_get_by_id(net, 1); sms->src.ton = submit->source_addr_ton; sms->src.npi = submit->source_addr_npi; strncpy(sms->src.addr, (char *)submit->source_addr, sizeof(sms->src.addr)-1); @@ -473,13 +472,13 @@ static int deliver_to_esme(struct osmo_esme *esme, struct gsm_sms *sms, deliver.source_addr_npi = NPI_Land_Mobile_E212; snprintf((char *)deliver.source_addr, sizeof(deliver.source_addr), "%s", - sms->sender->imsi); + conn->subscr->imsi); } else { deliver.source_addr_ton = TON_Network_Specific; deliver.source_addr_npi = NPI_ISDN_E163_E164; snprintf((char *)deliver.source_addr, sizeof(deliver.source_addr), "%s", - sms->sender->extension); + conn->subscr->extension); }
deliver.dest_addr_ton = sms->dst.ton;
The v4 DB scheme removes sender ID from the DB and stores individual values instead (sender addr, ton, npi). To convert an old DB to the new format we have to read all values from the old table and re-add them to the new one. --- openbsc/src/libmsc/db.c | 199 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 170 insertions(+), 29 deletions(-)
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 8f76324..d60f73e 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -41,8 +41,36 @@ static char *db_basename = NULL; static char *db_dirname = NULL; static dbi_conn conn; - -#define SCHEMA_REVISION "50000000" +static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result result); + +#define SCHEMA_REVISION "4" + +#define SMS_TABLE_CREATE_STMT \ + "CREATE TABLE IF NOT EXISTS SMS (" \ + /* metadata, not part of sms */ \ + "id INTEGER PRIMARY KEY AUTOINCREMENT, " \ + "created TIMESTAMP NOT NULL, " \ + "sent TIMESTAMP, " \ + "receiver_id INTEGER NOT NULL, " \ + "deliver_attempts INTEGER NOT NULL DEFAULT 0, " \ + /* data directly copied/derived from SMS */ \ + "valid_until TIMESTAMP, " \ + "reply_path_req INTEGER NOT NULL, " \ + "status_rep_req INTEGER NOT NULL, " \ + "protocol_id INTEGER NOT NULL, " \ + "data_coding_scheme INTEGER NOT NULL, " \ + "ud_hdr_ind INTEGER NOT NULL, " \ + "src_addr TEXT NOT NULL, " \ + "src_ton INTEGER NOT NULL, " \ + "src_npi INTEGER NOT NULL, " \ + "dest_addr TEXT NOT NULL, " \ + "dest_ton INTEGER NOT NULL, " \ + "dest_npi INTEGER NOT NULL, " \ + "user_data BLOB, " /* TP-UD */ \ + /* additional data, interpreted from SMS */ \ + "header BLOB, " /* UD Header */ \ + "text TEXT " /* decoded UD after UDH */ \ + ")"
static char *create_stmts[] = { "CREATE TABLE IF NOT EXISTS Meta (" @@ -90,31 +118,7 @@ static char *create_stmts[] = { "equipment_id NUMERIC NOT NULL, " "UNIQUE (subscriber_id, equipment_id) " ")", - "CREATE TABLE IF NOT EXISTS SMS (" - /* metadata, not part of sms */ - "id INTEGER PRIMARY KEY AUTOINCREMENT, " - "created TIMESTAMP NOT NULL, " - "sent TIMESTAMP, " - "receiver_id INTEGER NOT NULL, " - "deliver_attempts INTEGER NOT NULL DEFAULT 0, " - /* data directly copied/derived from SMS */ - "valid_until TIMESTAMP, " - "reply_path_req INTEGER NOT NULL, " - "status_rep_req INTEGER NOT NULL, " - "protocol_id INTEGER NOT NULL, " - "data_coding_scheme INTEGER NOT NULL, " - "ud_hdr_ind INTEGER NOT NULL, " - "src_addr TEXT NOT NULL, " - "src_ton INTEGER NOT NULL, " - "src_npi INTEGER NOT NULL, " - "dest_addr TEXT NOT NULL, " - "dest_ton INTEGER NOT NULL, " - "dest_npi INTEGER NOT NULL, " - "user_data BLOB, " /* TP-UD */ - /* additional data, interpreted from SMS */ - "header BLOB, " /* UD Header */ - "text TEXT " /* decoded UD after UDH */ - ")", + SMS_TABLE_CREATE_STMT, "CREATE TABLE IF NOT EXISTS VLR (" "id INTEGER PRIMARY KEY AUTOINCREMENT, " "created TIMESTAMP NOT NULL, " @@ -175,7 +179,7 @@ static int update_db_revision_2(void) "TIMESTAMP DEFAULT NULL"); if (!result) { LOGP(DDB, LOGL_ERROR, - "Failed to alter table Subscriber (upgrade vom rev 2).\n"); + "Failed to alter table Subscriber (upgrade from rev 2).\n"); return -EINVAL; } dbi_result_free(result); @@ -186,7 +190,138 @@ static int update_db_revision_2(void) "WHERE key = 'revision'"); if (!result) { LOGP(DDB, LOGL_ERROR, - "Failed set new revision (upgrade vom rev 2).\n"); + "Failed to update DB schema revision (upgrade from rev 2).\n"); + return -EINVAL; + } + dbi_result_free(result); + + return 0; +} + + +static struct gsm_sms *sms_from_result_v3(dbi_result result) +{ + struct gsm_sms *sms = sms_alloc(); + long long unsigned int sender_id; + struct gsm_subscriber *sender; + const char *text, *daddr; + const unsigned char *user_data; + char buf[32]; + + if (!sms) + return NULL; + + sms->id = dbi_result_get_ulonglong(result, "id"); + + sender_id = dbi_result_get_ulonglong(result, "sender_id"); + sprintf(buf, "%llu", sender_id); + sender = db_get_subscriber(NULL, GSM_SUBSCRIBER_ID, buf); + strncpy(sms->src.addr, sender->extension, sizeof(sms->src.addr)-1); + subscr_put(sender); + + sms->receiver = NULL; + + /* FIXME: validity */ + /* FIXME: those should all be get_uchar, but sqlite3 is braindead */ + sms->reply_path_req = dbi_result_get_uint(result, "reply_path_req"); + sms->status_rep_req = dbi_result_get_uint(result, "status_rep_req"); + sms->ud_hdr_ind = dbi_result_get_uint(result, "ud_hdr_ind"); + sms->protocol_id = dbi_result_get_uint(result, "protocol_id"); + sms->data_coding_scheme = dbi_result_get_uint(result, + "data_coding_scheme"); + /* sms->msg_ref is temporary and not stored in DB */ + + daddr = dbi_result_get_string(result, "dest_addr"); + if (daddr) { + strncpy(sms->dst.addr, daddr, sizeof(sms->dst.addr)); + sms->dst.addr[sizeof(sms->dst.addr)-1] = '\0'; + } + + sms->user_data_len = dbi_result_get_field_length(result, "user_data"); + user_data = dbi_result_get_binary(result, "user_data"); + if (sms->user_data_len > sizeof(sms->user_data)) + sms->user_data_len = (uint8_t) sizeof(sms->user_data); + memcpy(sms->user_data, user_data, sms->user_data_len); + + text = dbi_result_get_string(result, "text"); + if (text) { + strncpy(sms->text, text, sizeof(sms->text)); + sms->text[sizeof(sms->text)-1] = '\0'; + } + return sms; +} + +static int update_db_revision_3(void) +{ + dbi_result result; + struct gsm_sms *sms; + + /* Rename old SMS table to be able create a new one */ + result = dbi_conn_query(conn, + "ALTER TABLE SMS " + "RENAME TO SMS_3"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to rename the old SMS table (upgrade from rev 3).\n"); + return -EINVAL; + } + dbi_result_free(result); + + /* Create new SMS table with all the bells and whistles! */ + result = dbi_conn_query(conn, + SMS_TABLE_CREATE_STMT); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to create a new SMS table (upgrade from rev 3).\n"); + return -EINVAL; + } + dbi_result_free(result); + + /* Cycle through old messages and convert them to the new format */ + result = dbi_conn_queryf(conn, + "SELECT * FROM SMS_3"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed fetch messages from the old SMS table (upgrade from rev 3).\n"); + return -EINVAL; + } + while (dbi_result_next_row(result)) { + sms = sms_from_result_v3(result); + if (db_sms_store(sms) != 0) { + LOGP(DDB, LOGL_ERROR, "Failed to store message to the new SMS table(upgrade from rev 3).\n"); + } + sms_free(sms); + } + dbi_result_free(result); + + /* Mark SMS_3 table for removal */ + result = dbi_conn_query(conn, + "DROP TABLE SMS_3"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to drop the old SMS table (upgrade from rev 3).\n"); + return -EINVAL; + } + dbi_result_free(result); + + /* Shrink DB file size by actually wiping out SMS_3 table data */ + result = dbi_conn_query(conn, + "VACUUM"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed VACUUM after droping the old SMS table (upgrade from rev 3).\n"); + return -EINVAL; + } + dbi_result_free(result); + + /* We're done. Bump DB Meta revision to 4 */ + result = dbi_conn_query(conn, + "UPDATE Meta " + "SET value = '4' " + "WHERE key = 'revision'"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to update DB schema revision (upgrade from rev 3).\n"); return -EINVAL; } dbi_result_free(result); @@ -219,6 +354,12 @@ static int check_db_revision(void) dbi_result_free(result); return -EINVAL; } + } else if (!strcmp(rev_s, "3")) { + if (update_db_revision_3()) { + LOGP(DDB, LOGL_FATAL, "Failed to update database from schema revision '%s'.\n", rev_s); + dbi_result_free(result); + return -EINVAL; + } } else if (!strcmp(rev_s, SCHEMA_REVISION)) { /* everything is fine */ } else {
On Tue, Oct 08, 2013 at 03:17:31AM +0200, Alexander Chemeris wrote:
The v4 DB scheme removes sender ID from the DB and stores individual values instead (sender addr, ton, npi). To convert an old DB to the new format we have to read all values from the old table and re-add them to the new one.
we need to squash this with the previous commit and you should add line wrapping to your text as well.
+#define SMS_TABLE_CREATE_STMT \
Can you propose an alternative to move this out of the array?
"Failed to alter table Subscriber (upgrade vom rev 2).\n");
"Failed to alter table Subscriber (upgrade from rev 2).\n");
haha, german and with a typo. :)
- /* Rename old SMS table to be able create a new one */
- result = dbi_conn_query(conn,
"ALTER TABLE SMS ""RENAME TO SMS_3");
Okay, that is easier than adding a new column, populating it, removing the old one, changing the constraints. It is at the cost of having an additional parser routine.
- /* Mark SMS_3 table for removal */
How is it marked? ;)
- } else if (!strcmp(rev_s, "3")) {
if (update_db_revision_3()) {
FAILURE
reading it is odd, but this is how update_db_revision_2 is doing it.. you just copied it.
On Tue, Oct 8, 2013 at 12:01 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 03:17:31AM +0200, Alexander Chemeris wrote:
The v4 DB scheme removes sender ID from the DB and stores individual values instead (sender addr, ton, npi). To convert an old DB to the new format we have to read all values from the old table and re-add them to the new one.
we need to squash this with the previous commit and you should add line wrapping to your text as well.
Squashing will remove your copyright on the code, so I avoided that. I don't see an issue with having two separate commits.
+#define SMS_TABLE_CREATE_STMT \
Can you propose an alternative to move this out of the array?
Frankly speaking, I would move _all_ these statements to separate #defines, to make the code clearer.
And no, I don't see a better way to do that.
/* Rename old SMS table to be able create a new one */result = dbi_conn_query(conn,"ALTER TABLE SMS ""RENAME TO SMS_3");Okay, that is easier than adding a new column, populating it, removing the old one, changing the constraints. It is at the cost of having an additional parser routine.
The thin is that in SQLite you can't remove a column. So this is the only possible way.
/* Mark SMS_3 table for removal */How is it marked? ;)
It's not actually removed from the file. VACUUM is needed to actually remove the data. Thus "marked".
} else if (!strcmp(rev_s, "3")) {if (update_db_revision_3()) {FAILUREreading it is odd, but this is how update_db_revision_2 is doing it.. you just copied it.
Yes, I didn't want to change the code style.
On Tue, Oct 08, 2013 at 12:01:55PM +0200, Holger Hans Peter Freyther wrote:
Hi Alexander,
I would like to get this patchset included and I noticed that you have never followed up on this comment.
+#define SMS_TABLE_CREATE_STMT \
Can you propose an alternative to move this out of the array?
Couldn't you use ALTER TABLE from within a transaction?
kind regards holger
Hi Holger,
On Sat, Feb 8, 2014 at 6:42 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 12:01:55PM +0200, Holger Hans Peter Freyther wrote: I would like to get this patchset included and I noticed that you have never followed up on this comment.
The code for the DB conversion doesn't work in the submitted form. I fixed this, but I haven't had enough time to port the patch to master. I could submit it, if someone wants to port it before I find the time to do this myself. So far we've been working with jolly-branches, as only they support OsmoTRX and thus UmTRX.
On Sat, Feb 08, 2014 at 09:02:46PM +0400, Alexander Chemeris wrote:
Dear Alexander,
The code for the DB conversion doesn't work in the submitted form. I fixed this, but I haven't had enough time to port the patch to master. I could submit it, if someone wants to port it before I find the time to do this myself. So far we've been working with jolly-branches, as only they support OsmoTRX and thus UmTRX.
That is very sad to hear. I encourage you to finish your patch and the total number of patches of yours would raise to eight.
Please try to contribute and finish this patch or at least just make your modification available to the community.
holger
Hi Holger,
On Sun, Feb 9, 2014 at 11:25 AM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, Feb 08, 2014 at 09:02:46PM +0400, Alexander Chemeris wrote:
The code for the DB conversion doesn't work in the submitted form. I fixed this, but I haven't had enough time to port the patch to master. I could submit it, if someone wants to port it before I find the time to do this myself. So far we've been working with jolly-branches, as only they support OsmoTRX and thus UmTRX.
That is very sad to hear.
I'm glad that you share my sadness that jolly branches are not yet merged. I hope you'll find some time to continue reviewing the patchset for L1 parts, which jolly submitted quite a while ago.
I encourage you to finish your patch and the total number of patches of yours would raise to eight.
Thank you for beancounting for me :)
In Russia we say "Never count money in someone's else pocket", I think s/money/patches/ and s/pocket/contribution/ would make a great saying for open-source world.
As much as I love programming and contributing, at this moment I have to focus on enabling other people to contribute. I hope that one day I could give up my management tasks and dive deep into writing lots of code again. That said, I'm happy that Fairwaves is funding substantial amount of work on Osmocom, including OsmoPCU, OsmoTRX, UmTRX and a bit of OsmoBTS and OsmoNITB. And we're only increasing amounts of this funding, as well as attracting more community members.
Please try to contribute and finish this patch or at least just make your modification available to the community.
I realized, that this is actually the final working version of the patch. I took you so long to get back to it, so I was thinking it's the original patch which was broken. Feel free to merge this into master.
On Sun, Feb 09, 2014 at 01:28:05PM +0400, Alexander Chemeris wrote:
Hi,
I'm glad that you share my sadness that jolly branches are not yet merged. I hope you'll find some time to continue reviewing the patchset for L1 parts, which jolly submitted quite a while ago.
but as you pointed out in the other SMS email, he needs to add tests and things like (errno == 111) is an obvious no go.
As much as I love programming and contributing, at this moment I have to focus on enabling other people to contribute. I hope that one day I could give up my management tasks and dive deep into writing lots of code again. That said, I'm happy that Fairwaves is funding substantial amount of work on Osmocom, including OsmoPCU, OsmoTRX, UmTRX and a bit of OsmoBTS and OsmoNITB. And we're only increasing amounts of this funding, as well as attracting more community members.
Please stop the marketing here. But if you really want to help, maybe mentor Andreas and review his patches before it goes to the mailinglist. I have spent way too much time on this already, it is not economic for me to do his QA. I have cats to feed!
Please try to contribute and finish this patch or at least just make your modification available to the community.
I realized, that this is actually the final working version of the patch. I took you so long to get back to it, so I was thinking it's the original patch which was broken. Feel free to merge this into master.
It lacks a test case and the doubt if it works or not doesn't help either. But sure it is easy to demand things from others. ;)
holger
On Sun, Feb 9, 2014 at 1:48 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sun, Feb 09, 2014 at 01:28:05PM +0400, Alexander Chemeris wrote:
Please try to contribute and finish this patch or at least just make your modification available to the community.
I realized, that this is actually the final working version of the patch. I took you so long to get back to it, so I was thinking it's the original patch which was broken. Feel free to merge this into master.
It lacks a test case and the doubt if it works or not doesn't help either.
I doubt a unit-test is needed in this case, given the purpose of this code and that this code is more or less self-contained. If you want, you could try it yourself, but I have tested it on pretty large DB full of SMS messages.
But sure it is easy to demand things from others. ;)
I propose this to be the Rule #1 for this mailing list :) "Don't demand others to do work for you."
On Sun, Feb 09, 2014 at 02:13:38PM +0400, Alexander Chemeris wrote:
But sure it is easy to demand things from others. ;)
I propose this to be the Rule #1 for this mailing list :) "Don't demand others to do work for you."
If it makes you happy. But remember how this request started. ;)
Hi Holger,
On Sat, Feb 8, 2014 at 6:42 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 12:01:55PM +0200, Holger Hans Peter Freyther wrote:
Hi Alexander,
I would like to get this patchset included and I noticed that you have never followed up on this comment.
+#define SMS_TABLE_CREATE_STMT \
Can you propose an alternative to move this out of the array?
Couldn't you use ALTER TABLE from within a transaction?
No, ALTER TABLE can't be used, because it can only add column in SQLIte and can't remove them. The whole twist with this code is to workaround this limitation. It was discussed a while ago in the original topic.
Actually, I would propose to move all other statements in that array to #define's as well to make the code more readable.
On Sun, Feb 9, 2014 at 1:14 PM, Alexander Chemeris alexander.chemeris@gmail.com wrote:
Hi Holger,
On Sat, Feb 8, 2014 at 6:42 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 12:01:55PM +0200, Holger Hans Peter Freyther wrote:
Hi Alexander,
I would like to get this patchset included and I noticed that you have never followed up on this comment.
+#define SMS_TABLE_CREATE_STMT \
Can you propose an alternative to move this out of the array?
Couldn't you use ALTER TABLE from within a transaction?
No, ALTER TABLE can't be used, because it can only add column in SQLIte and can't remove them. The whole twist with this code is to workaround this limitation. It was discussed a while ago in the original topic.
Actually, I would propose to move all other statements in that array to #define's as well to make the code more readable.
Btw, I did reply to this question earlier in the thread. :) My original reply was:
Frankly speaking, I would move _all_ these statements to separate #defines, to make the code clearer.
And no, I don't see a better way to do that.
That was a bad idea from the very beginning. A visible result of this is a wrong SMS routing when you change subscriber extensions, while having queued SMS. It's also a very wrong thing from the code layering perspective.
I think the next logical step should be to remove "receiver" pointer from the gsm_sms structure into a structure, special for the internal SMS queue. --- openbsc/src/libmsc/db.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index d60f73e..2c77538 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -51,7 +51,6 @@ static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result resul "id INTEGER PRIMARY KEY AUTOINCREMENT, " \ "created TIMESTAMP NOT NULL, " \ "sent TIMESTAMP, " \ - "receiver_id INTEGER NOT NULL, " \ "deliver_attempts INTEGER NOT NULL DEFAULT 0, " \ /* data directly copied/derived from SMS */ \ "valid_until TIMESTAMP, " \ @@ -1256,19 +1255,19 @@ int db_sms_store(struct gsm_sms *sms) /* FIXME: correct validity period */ result = dbi_conn_queryf(conn, "INSERT INTO SMS " - "(created, receiver_id, valid_until, " + "(created, valid_until, " "reply_path_req, status_rep_req, protocol_id, " "data_coding_scheme, ud_hdr_ind, " "user_data, text, " "dest_addr, dest_ton, dest_npi, " "src_addr, src_ton, src_npi) VALUES " - "(datetime('now'), %llu, %u, " + "(datetime('now'), %u, " "%u, %u, %u, " "%u, %u, " "%s, %s, " "%s, %u, %u, " "%s, %u, %u)", - sms->receiver ? sms->receiver->id : 0, validity_timestamp, + validity_timestamp, sms->reply_path_req, sms->status_rep_req, sms->protocol_id, sms->data_coding_scheme, sms->ud_hdr_ind, q_udata, q_text, @@ -1289,7 +1288,6 @@ int db_sms_store(struct gsm_sms *sms) static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result result) { struct gsm_sms *sms = sms_alloc(); - long long unsigned int receiver_id; const char *text, *daddr, *saddr; const unsigned char *user_data;
@@ -1298,9 +1296,6 @@ static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result resul
sms->id = dbi_result_get_ulonglong(result, "id");
- receiver_id = dbi_result_get_ulonglong(result, "receiver_id"); - sms->receiver = subscr_get_by_id(net, receiver_id); - /* FIXME: validity */ /* FIXME: those should all be get_uchar, but sqlite3 is braindead */ sms->reply_path_req = dbi_result_get_uint(result, "reply_path_req"); @@ -1318,6 +1313,7 @@ static struct gsm_sms *sms_from_result(struct gsm_network *net, dbi_result resul strncpy(sms->dst.addr, daddr, sizeof(sms->dst.addr)); sms->dst.addr[sizeof(sms->dst.addr)-1] = '\0'; } + sms->receiver = subscr_get_by_extension(net, sms->dst.addr);
sms->src.npi = dbi_result_get_uint(result, "src_npi"); sms->src.ton = dbi_result_get_uint(result, "src_ton"); @@ -1372,7 +1368,7 @@ struct gsm_sms *db_sms_get_unsent(struct gsm_network *net, unsigned long long mi result = dbi_conn_queryf(conn, "SELECT SMS.* " "FROM SMS JOIN Subscriber ON " - "SMS.receiver_id = Subscriber.id " + "SMS.dest_addr = Subscriber.extension " "WHERE SMS.id >= %llu AND SMS.sent IS NULL " "AND Subscriber.lac > 0 " "ORDER BY SMS.id LIMIT 1", @@ -1402,10 +1398,10 @@ struct gsm_sms *db_sms_get_unsent_by_subscr(struct gsm_network *net, result = dbi_conn_queryf(conn, "SELECT SMS.* " "FROM SMS JOIN Subscriber ON " - "SMS.receiver_id = Subscriber.id " - "WHERE SMS.receiver_id >= %llu AND SMS.sent IS NULL " + "SMS.dest_addr = Subscriber.extension " + "WHERE Subscriber.id >= %llu AND SMS.sent IS NULL " "AND Subscriber.lac > 0 AND SMS.deliver_attempts < %u " - "ORDER BY SMS.receiver_id, SMS.id LIMIT 1", + "ORDER BY Subscriber.id, SMS.id LIMIT 1", min_subscr_id, failed); if (!result) return NULL; @@ -1431,8 +1427,8 @@ struct gsm_sms *db_sms_get_unsent_for_subscr(struct gsm_subscriber *subscr) result = dbi_conn_queryf(conn, "SELECT SMS.* " "FROM SMS JOIN Subscriber ON " - "SMS.receiver_id = Subscriber.id " - "WHERE SMS.receiver_id = %llu AND SMS.sent IS NULL " + "SMS.dest_addr = Subscriber.extension " + "WHERE Subscriber.id = %llu AND SMS.sent IS NULL " "AND Subscriber.lac > 0 " "ORDER BY SMS.id LIMIT 1", subscr->id);
On Tue, Oct 08, 2013 at 03:17:32AM +0200, Alexander Chemeris wrote:
That was a bad idea from the very beginning. A visible result of this is a wrong SMS routing when you change subscriber extensions, while having queued SMS. It's also a very wrong thing from the code layering perspective.
I think the next logical step should be to remove "receiver" pointer from the gsm_sms structure into a structure, special for the internal SMS queue.
how much extra effort is it to remove ->receiver right now?
On Tue, Oct 8, 2013 at 12:03 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 03:17:32AM +0200, Alexander Chemeris wrote:
That was a bad idea from the very beginning. A visible result of this is a wrong SMS routing when you change subscriber extensions, while having queued SMS. It's also a very wrong thing from the code layering perspective.
I think the next logical step should be to remove "receiver" pointer from the gsm_sms structure into a structure, special for the internal SMS queue.
how much extra effort is it to remove ->receiver right now?
We should create a new structure, new functions to create/free/populate it, and translate sms_queue.c to using it.
I think about a day of work for me. A bit more, than I could afford right now, unfortunately. :( May be a good task for someone who wants to contribute and doesn't know where to start.
On Tue, Oct 8, 2013 at 2:08 PM, Alexander Chemeris alexander.chemeris@gmail.com wrote:
On Tue, Oct 8, 2013 at 12:03 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 03:17:32AM +0200, Alexander Chemeris wrote:
That was a bad idea from the very beginning. A visible result of this is a wrong SMS routing when you change subscriber extensions, while having queued SMS. It's also a very wrong thing from the code layering perspective.
I think the next logical step should be to remove "receiver" pointer from the gsm_sms structure into a structure, special for the internal SMS queue.
how much extra effort is it to remove ->receiver right now?
We should create a new structure, new functions to create/free/populate it, and translate sms_queue.c to using it.
I looked into the code more closely and actually we already have a gsm_sms_pending structure in the SMS queue implementation, which also holds a pointer to the receiver subscriber. So the task is trivial from the architecture perspective - just move from using the pointer in the gsm_sms structure to using the pointer in gsm_sms_pending structure. But this requires careful thought and thus still quite time consuming.
On the similar venue, I'm thinking about delivering SMS to SMPP over unstable links (e.g. satellite), where the link may be inaccessible for non-negligible amounts of time. To make this usable, we need a queue in front of the SMPP interface. I see two options - introducing a dedicated queue for SMPP and modifying existing sms_queue to support delivery not only to local subscribers, but to SMPP and other future transports as well. I would be glad to know opinions on this.
Hi Alexander,
a bit of a late response, but
On the similar venue, I'm thinking about delivering SMS to SMPP over unstable links (e.g. satellite), where the link may be inaccessible for non-negligible amounts of time. To make this usable, we need a queue in front of the SMPP interface.
I would appreciate if that could be handled outside osmo-nitb by something like a stand-alone SMPP caching proxy. This proxy would behave as an SMPP ESMSE, accept any MO-SMS from the NITB, cache it locally and postpone delivery to the real ESME. This way OsmoNITB doesn't have to add code specifically for that situation.
The test fails, though, because we have removed a mock up version of paging_request(). I'm not sure how to properly fix the test, as the way it used to work is a pure hack. --- openbsc/tests/channel/Makefile.am | 4 +++- openbsc/tests/channel/channel_test.c | 12 ------------ 2 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/openbsc/tests/channel/Makefile.am b/openbsc/tests/channel/Makefile.am index 89f015a..a626ccf 100644 --- a/openbsc/tests/channel/Makefile.am +++ b/openbsc/tests/channel/Makefile.am @@ -8,5 +8,7 @@ noinst_PROGRAMS = channel_test channel_test_SOURCES = channel_test.c channel_test_LDADD = -ldl $(LIBOSMOCORE_LIBS) \ $(top_builddir)/src/libcommon/libcommon.a \ + $(top_builddir)/src/libmsc/libmsc.a \ + $(top_builddir)/src/libtrau/libtrau.a \ $(top_builddir)/src/libbsc/libbsc.a \ - $(top_builddir)/src/libmsc/libmsc.a -ldbi $(LIBOSMOGSM_LIBS) + -ldbi $(LIBOSMOGSM_LIBS) $(LIBOSMOABIS_LIBS) diff --git a/openbsc/tests/channel/channel_test.c b/openbsc/tests/channel/channel_test.c index ab0d9eb..0d50b08 100644 --- a/openbsc/tests/channel/channel_test.c +++ b/openbsc/tests/channel/channel_test.c @@ -45,12 +45,6 @@ static int subscr_cb(unsigned int hook, unsigned int event, struct msgb *msg, vo return 0; }
-/* mock object for testing, directly invoke the cb... maybe later through the timer */ -void paging_request(struct gsm_bts *bts, struct gsm_subscriber *subscriber, int type, gsm_cbfn *cbfn, void *data) -{ - cbfn(101, 200, (void*)0x1323L, &s_conn, data); -} -
int main(int argc, char **argv) { @@ -83,13 +77,7 @@ int main(int argc, char **argv) return EXIT_SUCCESS; }
-void _abis_nm_sendmsg() {} -void sms_alloc() {} -void gsm_net_update_ctype(struct gsm_network *network) {} -void gsm48_secure_channel() {} -void paging_request_stop() {} void vty_out() {} -void* connection_for_subscr(void) { abort(); return NULL; }
struct tlv_definition nm_att_tlvdef;
On Tue, Oct 08, 2013 at 03:17:33AM +0200, Alexander Chemeris wrote:
The test fails, though, because we have removed a mock up version of paging_request(). I'm not sure how to properly fix the test, as the way it used to work is a pure hack.
Can you elaborate when this patch will become necessary? From looking at your patches I don't see where it would be the case.
thanks holger
On Tue, Oct 8, 2013 at 12:05 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Tue, Oct 08, 2013 at 03:17:33AM +0200, Alexander Chemeris wrote:
The test fails, though, because we have removed a mock up version of paging_request(). I'm not sure how to properly fix the test, as the way it used to work is a pure hack.
Can you elaborate when this patch will become necessary? From looking at your patches I don't see where it would be the case.
I don't see why it should break either, but it does. Actually I don't understand how it worked before. Mock up version of paging_request() should be conflicting with the built-in function of the same name (this is what happens now). But for some reason, it didn't conflict before.
-
Alexander Chemeris -
Harald Welte -
Holger Hans Peter Freyther