I just fixed some things (gsm_subscriber.c, VTY tmsi parsing) and did some tests. It works!
Location Update Request with TMSI from previous network: <0002> gsm_04_08.c:1127 LOCATION UPDATING REQUEST: MI(TMSI)=0x68044a6c type=NORMAL <0001> gsm_04_08.c:144 (bts 0 trx 0 ts 0 pd 05) Sending 0x18 to MS. <0001> gsm_04_08.c:144 (bts 0 trx 0 ts 0 pd 05) Sending 0x18 to MS.
Outgoing SMS from OpenBSC VTY: OpenBSC> subscriber tmsi 0xb7f861b6 sms sender tmsi 0xb7f861b6 send TEST <0002> gsm_subscriber.c:175 Subscriber <IMSI> not paged yet. <0004> abis_rsl.c:1465 (bts=0,trx=0,ts=0,ss=0) Activating ARFCN(***) SS(0) lctype SDCCH r=OTHER ra=0x1a ta=1 <0004> abis_rsl.c:1199 (bts=0,trx=0,ts=0,ss=0) CHANNEL ACTIVATE ACK <0000> abis_rsl.c:1653 (bts=0,trx=0,ts=0,ss=0) SAPI=0 ESTABLISH INDICATION <0000> gsm_04_08.c:3573 Dispatching 04.08 message, pdisc=6 <0003> gsm_04_08.c:1180 PAGING RESPONSE: MI(TMSI)=0xb7f861b6 <0003> gsm_04_08.c:1198 <- Channel was requested by <IMSI> <0003> gsm_04_08.c:1259 TX APPLICATION INFO id=0x00, len=4 <0001> gsm_04_08.c:144 (bts 0 trx 0 ts 0 pd 06) Sending 0x38 to MS. <0001> transaction.c:71 subscr=0x2363f00, net=0x2333e90
USSD request also works: <0002> gsm_04_08.c:956 <- CM SERVICE REQUEST serv_type=0x08 MI(TMSI)=0xb7f861b6 <0002> gsm_04_08_utils.c:692 -> CM SERVICE ACK
As you can see, now TMSI displays in hex. I'll provide a new patch soon.
С наилучшими пожеланиями, Яницкий Вадим.
2016-03-22 18:58 GMT+06:00 Вадим Яницкий axilirator@gmail.com:
Hello, Harald! Good day, Holger!
It looks like SQLite3 doesn't have support of unsigned 64-bit integers. :( Of course, we can write some custom functions, which can emulate it, but it isn't good solution, I think.
My suggestion is to keep the TMSI column type in string format: 0xffffffff. I need to know your opinions before starting to write a new patch.
С наилучшими пожеланиями, Яницкий Вадим.
2016-03-17 22:34 GMT+06:00 Holger Freyther holger@freyther.de:
On 17 Mar 2016, at 17:21, Вадим Яницкий axilirator@gmail.com wrote:
Hi Guys!
If you have time, please check if there are other occurrences in
OpenBSC
or OsmocomBB where the TMSI is printed as integer. Thanks!
No problem! :)
I think it was a bit too quick. I foresee one problem. Let's assume
someone
is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the
migration/mitigation plan?
I absolutely agree with Holger. Maybe we can add some code that will
check
if database still stores TMSIs in old representation style and convert
them to
uint32_t? We can change the libmsc/db.c:db_prepare() for this purpose.
yes, we have a schema version and can just increase it. E.g. have a look at how we migrate SMS.
tmsi_from_string will not work for this anymore.
Yes, I forgot to change the gsm_subscriber.c ... Sorry.
it happens. thanks for contributing
there is no length check but that doesn't seem to be a big issue
right now.
We can just write a function that will do this check instead of using
#define.
I think you will not need to touch this define at all. We might want to change the name to _from_mi_string.
- a DB schema upgrade and store the TMSI as uint32_t
- Use hex presentation in VTY
+1
looking forward for the follow up.
holger
In OpenBSC, we traditionally displayed a TMSI in its integer representation, which is quite unusual in the telecom world. A TMSI is normally printed as a series of 8 hex digits.
This patch aligns OpenBSC with the telecom industry standard.
- Use hex representation in VTY - Increased DB SCHEMA_REVISION - Implemented DB migration code
Signed-off-by: Vadim Yanitskiy axilirator@gmail.com --- openbsc/include/openbsc/gsm_subscriber.h | 6 +- openbsc/src/libcommon/gsm_subscriber_base.c | 5 +- openbsc/src/libmsc/db.c | 96 ++++++++++++++++++++++++++--- openbsc/src/libmsc/gsm_04_08.c | 30 +++------ openbsc/src/libmsc/gsm_subscriber.c | 10 ++- openbsc/src/libmsc/vty_interface_layer3.c | 2 +- openbsc/src/osmo-bsc/osmo_bsc_filter.c | 2 +- openbsc/src/osmo-bsc/osmo_bsc_vty.c | 4 +- openbsc/tests/db/db_test.c | 4 +- openbsc/tests/gsm0408/gsm0408_test.c | 2 +- 10 files changed, 115 insertions(+), 46 deletions(-)
diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h index 7d6c776..de80530 100644 --- a/openbsc/include/openbsc/gsm_subscriber.h +++ b/openbsc/include/openbsc/gsm_subscriber.h @@ -14,7 +14,7 @@
#define GSM_SUBSCRIBER_FIRST_CONTACT 0x00000001 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */ -#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
#define GSM_SUBSCRIBER_NO_EXPIRATION 0x0
@@ -93,7 +93,7 @@ struct gsm_subscriber *subscr_put(struct gsm_subscriber *subscr); struct gsm_subscriber *subscr_create_subscriber(struct gsm_subscriber_group *sgrp, const char *imsi); struct gsm_subscriber *subscr_get_by_tmsi(struct gsm_subscriber_group *sgrp, - uint32_t tmsi); + const char *tmsi); struct gsm_subscriber *subscr_get_by_imsi(struct gsm_subscriber_group *sgrp, const char *imsi); struct gsm_subscriber *subscr_get_by_extension(struct gsm_subscriber_group *sgrp, @@ -104,7 +104,7 @@ struct gsm_subscriber *subscr_get_or_create(struct gsm_subscriber_group *sgrp, const char *imsi); int subscr_update(struct gsm_subscriber *s, struct gsm_bts *bts, int reason); struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group *sgrp, - uint32_t tmsi); + const char *tmsi); struct gsm_subscriber *subscr_active_by_imsi(struct gsm_subscriber_group *sgrp, const char *imsi);
diff --git a/openbsc/src/libcommon/gsm_subscriber_base.c b/openbsc/src/libcommon/gsm_subscriber_base.c index a455824..c7fb831 100644 --- a/openbsc/src/libcommon/gsm_subscriber_base.c +++ b/openbsc/src/libcommon/gsm_subscriber_base.c @@ -118,12 +118,13 @@ struct gsm_subscriber *subscr_get_or_create(struct gsm_subscriber_group *sgrp, }
struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group *sgrp, - uint32_t tmsi) + const char *tmsi) { struct gsm_subscriber *subscr; + uint8_t tmsi_val = tmsi_from_string(tmsi);
llist_for_each_entry(subscr, subscr_bsc_active_subscribers(), entry) { - if (subscr->tmsi == tmsi && subscr->group == sgrp) + if (subscr->tmsi == tmsi_val && subscr->group == sgrp) return subscr_get(subscr); }
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 0935fc5..56b5a08 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -47,7 +47,7 @@ static char *db_basename = NULL; static char *db_dirname = NULL; static dbi_conn conn;
-#define SCHEMA_REVISION "4" +#define SCHEMA_REVISION "5"
enum { SCHEMA_META, @@ -212,6 +212,7 @@ static int update_db_revision_2(void) } dbi_result_free(result);
+ LOGP(DDB, LOGL_NOTICE, "Migration complete.\n"); return 0; }
@@ -357,6 +358,7 @@ static int update_db_revision_3(void) else dbi_result_free(result);
+ LOGP(DDB, LOGL_NOTICE, "Migration complete.\n"); return 0;
rollback: @@ -369,6 +371,77 @@ rollback: return -EINVAL; }
+static int update_db_revision_4(void) +{ + dbi_result select; + dbi_result update; + long long unsigned int id; + const char *tmsi_old; + uint32_t tmsi_new; + + LOGP(DDB, LOGL_NOTICE, "Going to migrate from revision 4\n"); + + /* Cycle through old TMSIs and convert them to the new format */ + select = dbi_conn_query(conn, "SELECT * FROM Subscriber"); + if (!select) { + LOGP(DDB, LOGL_ERROR, + "Failed fetch subscriber data the old Subscriber table " + "(upgrade from rev 4).\n"); + return -EINVAL; + } + + while (dbi_result_next_row(select)) { + /* Fetch the subscriber ID */ + id = dbi_result_get_ulonglong(select, "id"); + + /* Fetch an old TMSI value */ + tmsi_old = dbi_result_get_string(select, "tmsi"); + tmsi_new = atoi(tmsi_old); + + if (tmsi_new <= 0) { + LOGP(DDB, LOGL_ERROR, + "Failed to convert an old TMSI '%s', ignoring " + "(upgrade from rev 4).\n", tmsi_old); + continue; + } + + /* Update old TMSI */ + update = dbi_conn_queryf(conn, + "UPDATE Subscriber " + "SET tmsi = '0x%08x' " + "WHERE id = %llu", + tmsi_new, id); + + if (!update) { + LOGP(DDB, LOGL_ERROR, + "Failed update subscriber's TMSI " + "(upgrade from rev 4).\n"); + + dbi_result_free(select); + return -EINVAL; + } + + dbi_result_free(update); + } + dbi_result_free(select); + + /* We're done. Bump DB Meta revision to 5 */ + update = dbi_conn_query(conn, + "UPDATE Meta " + "SET value = '5' " + "WHERE key = 'revision'"); + if (!update) { + LOGP(DDB, LOGL_ERROR, + "Failed to update DB schema revision " + "(upgrade from rev 4).\n"); + return -EINVAL; + } + + dbi_result_free(update); + LOGP(DDB, LOGL_NOTICE, "Migration complete.\n"); + return 0; +} + static int check_db_revision(void) { dbi_result result; @@ -400,6 +473,12 @@ static int check_db_revision(void) dbi_result_free(result); return -EINVAL; } + } else if (!strcmp(rev_s, "4")) { + if (update_db_revision_4()) { + 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 { @@ -893,9 +972,10 @@ struct gsm_subscriber *db_get_subscriber(enum gsm_subscriber_field field, subscr->id = dbi_result_get_ulonglong(result, "id");
db_set_from_query(subscr, result); - DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', TMSI %u, EXTEN '%s', LAC %hu, AUTH %u\n", - subscr->id, subscr->imsi, subscr->name, subscr->tmsi, subscr->extension, - subscr->lac, subscr->authorized); + DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', " + "TMSI 0x%08x, EXTEN '%s', LAC %hu, AUTH %u\n", + subscr->id, subscr->imsi, subscr->name, subscr->tmsi, + subscr->extension, subscr->lac, subscr->authorized); dbi_result_free(result);
get_equipment_by_subscr(subscr); @@ -935,7 +1015,7 @@ int db_subscriber_update(struct gsm_subscriber *subscr) int db_sync_subscriber(struct gsm_subscriber *subscriber) { dbi_result result; - char tmsi[14]; + char tmsi[11]; char *q_tmsi, *q_name, *q_extension;
dbi_conn_quote_string_copy(conn, @@ -944,7 +1024,7 @@ int db_sync_subscriber(struct gsm_subscriber *subscriber) subscriber->extension, &q_extension); if (subscriber->tmsi != GSM_RESERVED_TMSI) { - sprintf(tmsi, "%u", subscriber->tmsi); + sprintf(tmsi, "0x%08x", subscriber->tmsi); dbi_conn_quote_string_copy(conn, tmsi, &q_tmsi); @@ -1194,7 +1274,7 @@ int db_subscriber_expire(void *priv, void (*callback)(void *priv, long long unsi int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) { dbi_result result = NULL; - char tmsi[14]; + char tmsi[11]; char *tmsi_quoted;
for (;;) { @@ -1205,7 +1285,7 @@ int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) if (subscriber->tmsi == GSM_RESERVED_TMSI) continue;
- sprintf(tmsi, "%u", subscriber->tmsi); + sprintf(tmsi, "0x%08x", subscriber->tmsi); dbi_conn_quote_string_copy(conn, tmsi, &tmsi_quoted); result = dbi_conn_queryf(conn, "SELECT * FROM Subscriber " diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 1524ec4..c9f1b66 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -653,8 +653,7 @@ static int mm_rx_loc_upd_req(struct gsm_subscriber_connection *conn, struct msgb case GSM_MI_TYPE_TMSI: DEBUGPC(DMM, "\n"); /* look up the subscriber based on TMSI, request IMSI if it fails */ - subscr = subscr_get_by_tmsi(bts->network->subscr_group, - tmsi_from_string(mi_string)); + subscr = subscr_get_by_tmsi(bts->network->subscr_group, mi_string); if (!subscr) { /* send IDENTITY REQUEST message to get IMSI */ mm_tx_identity_req(conn, GSM_MI_TYPE_IMSI); @@ -972,20 +971,15 @@ static int gsm48_rx_mm_serv_req(struct gsm_subscriber_connection *conn, struct m
if (mi_type == GSM_MI_TYPE_IMSI) { DEBUGPC(DMM, "serv_type=0x%02x MI(%s)=%s\n", - req->cm_service_type, gsm48_mi_type_name(mi_type), - mi_string); - subscr = subscr_get_by_imsi(bts->network->subscr_group, - mi_string); + req->cm_service_type, gsm48_mi_type_name(mi_type), mi_string); + subscr = subscr_get_by_imsi(bts->network->subscr_group, mi_string); } else if (mi_type == GSM_MI_TYPE_TMSI) { DEBUGPC(DMM, "serv_type=0x%02x MI(%s)=%s\n", - req->cm_service_type, gsm48_mi_type_name(mi_type), - mi_string); - subscr = subscr_get_by_tmsi(bts->network->subscr_group, - tmsi_from_string(mi_string)); + req->cm_service_type, gsm48_mi_type_name(mi_type), mi_string); + subscr = subscr_get_by_tmsi(bts->network->subscr_group, mi_string); } else { DEBUGPC(DMM, "mi_type is not expected: %d\n", mi_type); - return gsm48_tx_mm_serv_rej(conn, - GSM48_REJECT_INCORRECT_MESSAGE); + return gsm48_tx_mm_serv_rej(conn, GSM48_REJECT_INCORRECT_MESSAGE); }
osmo_signal_dispatch(SS_SUBSCR, S_SUBSCR_IDENTITY, (classmark2 + classmark2_len)); @@ -1038,13 +1032,11 @@ static int gsm48_rx_mm_imsi_detach_ind(struct gsm_subscriber_connection *conn, s switch (mi_type) { case GSM_MI_TYPE_TMSI: DEBUGPC(DMM, "\n"); - subscr = subscr_get_by_tmsi(bts->network->subscr_group, - tmsi_from_string(mi_string)); + subscr = subscr_get_by_tmsi(bts->network->subscr_group, mi_string); break; case GSM_MI_TYPE_IMSI: DEBUGPC(DMM, "\n"); - subscr = subscr_get_by_imsi(bts->network->subscr_group, - mi_string); + subscr = subscr_get_by_imsi(bts->network->subscr_group, mi_string); break; case GSM_MI_TYPE_IMEI: case GSM_MI_TYPE_IMEISV: @@ -1189,12 +1181,10 @@ static int gsm48_rx_rr_pag_resp(struct gsm_subscriber_connection *conn, struct m
switch (mi_type) { case GSM_MI_TYPE_TMSI: - subscr = subscr_get_by_tmsi(bts->network->subscr_group, - tmsi_from_string(mi_string)); + subscr = subscr_get_by_tmsi(bts->network->subscr_group, mi_string); break; case GSM_MI_TYPE_IMSI: - subscr = subscr_get_by_imsi(bts->network->subscr_group, - mi_string); + subscr = subscr_get_by_imsi(bts->network->subscr_group, mi_string); break; }
diff --git a/openbsc/src/libmsc/gsm_subscriber.c b/openbsc/src/libmsc/gsm_subscriber.c index 57c10cf..d9761d9 100644 --- a/openbsc/src/libmsc/gsm_subscriber.c +++ b/openbsc/src/libmsc/gsm_subscriber.c @@ -46,7 +46,6 @@ extern struct llist_head *subscr_bsc_active_subscribers(void); int gsm48_secure_channel(struct gsm_subscriber_connection *conn, int key_seq, gsm_cbfn *cb, void *cb_data);
- /* * Struct for pending channel requests. This is managed in the * llist_head requests of each subscriber. The reference counting @@ -212,19 +211,18 @@ struct gsm_subscriber *subscr_create_subscriber(struct gsm_subscriber_group *sgr }
struct gsm_subscriber *subscr_get_by_tmsi(struct gsm_subscriber_group *sgrp, - uint32_t tmsi) + const char *tmsi) { - char tmsi_string[14]; struct gsm_subscriber *subscr; + uint32_t tmsi_val = tmsi_from_string(tmsi);
/* we might have a record in memory already */ llist_for_each_entry(subscr, subscr_bsc_active_subscribers(), entry) { - if (tmsi == subscr->tmsi) + if (tmsi_val == subscr->tmsi) return subscr_get(subscr); }
- sprintf(tmsi_string, "%u", tmsi); - return get_subscriber(sgrp, GSM_SUBSCRIBER_TMSI, tmsi_string); + return get_subscriber(sgrp, GSM_SUBSCRIBER_TMSI, tmsi); }
struct gsm_subscriber *subscr_get_by_imsi(struct gsm_subscriber_group *sgrp, diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index f49c53a..19c15e1 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -189,7 +189,7 @@ static struct gsm_subscriber *get_subscr_by_argv(struct gsm_network *gsmnet, else if (!strcmp(type, "imsi")) return subscr_get_by_imsi(gsmnet->subscr_group, id); else if (!strcmp(type, "tmsi")) - return subscr_get_by_tmsi(gsmnet->subscr_group, atoi(id)); + return subscr_get_by_tmsi(gsmnet->subscr_group, id); else if (!strcmp(type, "id")) return subscr_get_by_id(gsmnet->subscr_group, atoi(id));
diff --git a/openbsc/src/osmo-bsc/osmo_bsc_filter.c b/openbsc/src/osmo-bsc/osmo_bsc_filter.c index 14e0b71..88db15e 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_filter.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_filter.c @@ -79,7 +79,7 @@ static struct gsm_subscriber *extract_sub(struct gsm_subscriber_connection *conn switch (mi_type) { case GSM_MI_TYPE_TMSI: subscr = subscr_active_by_tmsi(conn->bts->network->subscr_group, - tmsi_from_string(mi_string)); + mi_string); break; case GSM_MI_TYPE_IMSI: subscr = subscr_active_by_imsi(conn->bts->network->subscr_group, diff --git a/openbsc/src/osmo-bsc/osmo_bsc_vty.c b/openbsc/src/osmo-bsc/osmo_bsc_vty.c index e623c9c..d871f01 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_vty.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_vty.c @@ -43,7 +43,7 @@ static struct osmo_bsc_data *osmo_bsc_data(struct vty *vty)
static struct osmo_msc_data *osmo_msc_data(struct vty *vty) { - return vty->index; + return osmo_msc_data_find(bsc_gsmnet, (int) vty->index); }
static struct cmd_node bsc_node = { @@ -70,7 +70,7 @@ DEFUN(cfg_net_msc, cfg_net_msc_cmd, return CMD_WARNING; }
- vty->index = msc; + vty->index = (void *) index; vty->node = MSC_NODE; return CMD_SUCCESS; } diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c index a02d1f8..faea820 100644 --- a/openbsc/tests/db/db_test.c +++ b/openbsc/tests/db/db_test.c @@ -200,7 +200,7 @@ int main() alice->lac=42; db_sync_subscriber(alice); /* Get by TMSI */ - snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + snprintf(scratch_str, sizeof(scratch_str), "0x%08x", alice->tmsi); alice_db = db_get_subscriber(GSM_SUBSCRIBER_TMSI, scratch_str); COMPARE(alice, alice_db); SUBSCR_PUT(alice_db); @@ -227,7 +227,7 @@ int main() db_subscriber_assoc_imei(alice, "1234567890"); db_subscriber_assoc_imei(alice, "6543560920"); /* Get by TMSI */ - snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + snprintf(scratch_str, sizeof(scratch_str), "0x%08x", alice->tmsi); alice_db = db_get_subscriber(GSM_SUBSCRIBER_TMSI, scratch_str); COMPARE(alice, alice_db); SUBSCR_PUT(alice_db); diff --git a/openbsc/tests/gsm0408/gsm0408_test.c b/openbsc/tests/gsm0408/gsm0408_test.c index 781ef61..8ed57ca 100644 --- a/openbsc/tests/gsm0408/gsm0408_test.c +++ b/openbsc/tests/gsm0408/gsm0408_test.c @@ -93,7 +93,7 @@ static void test_mi_functionality(void) /* tmsi code */ mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi); gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2); - COMPARE((uint32_t)strtoul(mi_parsed, NULL, 10), ==, tmsi); + COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi);
/* imsi code */ mi_len = gsm48_generate_mid_from_imsi(mi, imsi_odd);
On 27 Mar 2016, at 14:17, Vadim Yanitskiy axilirator@gmail.com wrote:
Dear Vadim,
In OpenBSC, we traditionally displayed a TMSI in its integer representation, which is quite unusual in the telecom world. A TMSI is normally printed as a series of 8 hex digits.
This patch aligns OpenBSC with the telecom industry standard.
thanks a lot, I am afraid we need one more round.
-#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
this macro is used for parsing strings from the network. We should not modify it.
struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group *sgrp,
uint32_t tmsi)
const char *tmsi)
why? the number of bytes needed fit in uint32_t so we should remain with this internal storage. We should just make sure to always print it as hex.
-#define SCHEMA_REVISION "4" +#define SCHEMA_REVISION "5"
good, but I think we should change the schema to use uint32_t/INTEGER for the TMSI instead of text.
/* Update old TMSI */update = dbi_conn_queryf(conn,"UPDATE Subscriber ""SET tmsi = '0x%08x' ""WHERE id = %llu",
but to int :)
kind regards holger
Hello!
thanks a lot, I am afraid we need one more round.
No problem :)
-#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
this macro is used for parsing strings from the network. We should not
modify it.
I cannot find where...
Searching 420 files for "tmsi_from_string" (regex)
/home/wmn/openbsc/openbsc/include/openbsc/gsm_subscriber.h: 15 #define GSM_SUBSCRIBER_FIRST_CONTACT 0x00000001 16 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */ 17: #define tmsi_from_string(str) strtoul(str + 2, NULL, 16) 18 19 #define GSM_SUBSCRIBER_NO_EXPIRATION 0x0
/home/wmn/openbsc/openbsc/src/libcommon/gsm_subscriber_base.c: 122 { 123 struct gsm_subscriber *subscr; 124: uint8_t tmsi_val = tmsi_from_string(tmsi); 125 126 llist_for_each_entry(subscr, subscr_bsc_active_subscribers(), entry) {
/home/wmn/openbsc/openbsc/src/libmsc/db.c: 890 string = dbi_result_get_string(result, "tmsi"); 891 if (string) 892: subscr->tmsi = tmsi_from_string(string); 893 894 string = dbi_result_get_string(result, "name");
/home/wmn/openbsc/openbsc/src/libmsc/gsm_subscriber.c: 215 { 216 struct gsm_subscriber *subscr; 217: uint32_t tmsi_val = tmsi_from_string(tmsi); 218 219 /* we might have a record in memory already */
/home/wmn/openbsc/openbsc/tests/gsm0408/gsm0408_test.c: 94 mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi); 95 gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2); 96: COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi); 97 98 /* imsi code */
5 matches across 5 files
struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group
*sgrp,
uint32_t tmsi)
const char *tmsi)why? the number of bytes needed fit in uint32_t so we should remain with
this internal storage. We should just make sure to always print it as hex.
Ok, I'll revert this changes.
-#define SCHEMA_REVISION "4" +#define SCHEMA_REVISION "5"
good, but I think we should change the schema to use uint32_t/INTEGER for
the TMSI instead of text.
I'll do it in db.c.
С наилучшими пожеланиями, Яницкий Вадим.
2016-03-27 22:44 GMT+06:00 Holger Freyther holger@freyther.de:
On 27 Mar 2016, at 14:17, Vadim Yanitskiy axilirator@gmail.com wrote:
Dear Vadim,
In OpenBSC, we traditionally displayed a TMSI in its integer representation, which is quite unusual in the telecom world. A TMSI is normally printed as a series of 8 hex digits.
This patch aligns OpenBSC with the telecom industry standard.
thanks a lot, I am afraid we need one more round.
-#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
this macro is used for parsing strings from the network. We should not modify it.
struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group
*sgrp,
uint32_t tmsi)
const char *tmsi)why? the number of bytes needed fit in uint32_t so we should remain with this internal storage. We should just make sure to always print it as hex.
-#define SCHEMA_REVISION "4" +#define SCHEMA_REVISION "5"
good, but I think we should change the schema to use uint32_t/INTEGER for the TMSI instead of text.
/* Update old TMSI */update = dbi_conn_queryf(conn,"UPDATE Subscriber ""SET tmsi = '0x%08x' ""WHERE id = %llu",but to int :)
kind regards holger
On 29 Mar 2016, at 18:10, Вадим Яницкий axilirator@gmail.com wrote:
Hello!
thanks a lot, I am afraid we need one more round.
No problem :)
-#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
this macro is used for parsing strings from the network. We should not modify it.
I cannot find where...
ah I see. You change the invocations in gsm_04_08.c. I would prefer if the name of this method is changed but once we work with uint32_t it will mostly go away anyway?
holger
Yes, we needn't this macro now. The only place it can be used is vty_interface_layer3.c:
static struct gsm_subscriber *get_subscr_by_argv(struct gsm_network *gsmnet, const char *type, const char *id) { if (!strcmp(type, "extension")) return subscr_get_by_extension(gsmnet->subscr_group, id); else if (!strcmp(type, "imsi")) return subscr_get_by_imsi(gsmnet->subscr_group, id); else if (!strcmp(type, "tmsi")) return subscr_get_by_tmsi(gsmnet->subscr_group, id); else if (!strcmp(type, "id")) return subscr_get_by_id(gsmnet->subscr_group, atoi(id));
return NULL; }
In this place we have to convert a string (written from VTY) to uint32_t and then call the subscr_get_by_tmsi() with converted value. User input can be unexpected, so we should check/limit the length and check if there is '0x' sequence or not.
С наилучшими пожеланиями, Яницкий Вадим.
2016-03-29 22:39 GMT+06:00 Holger Freyther holger@freyther.de:
On 29 Mar 2016, at 18:10, Вадим Яницкий axilirator@gmail.com wrote:
Hello!
thanks a lot, I am afraid we need one more round.
No problem :)
-#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
this macro is used for parsing strings from the network. We should not
modify it.
I cannot find where...
ah I see. You change the invocations in gsm_04_08.c. I would prefer if the name of this method is changed but once we work with uint32_t it will mostly go away anyway?
holger
In OpenBSC, we traditionally displayed a TMSI in its integer representation, which is quite unusual in the telecom world. A TMSI is normally printed as a series of 8 hex digits.
This patch aligns OpenBSC with the telecom industry standard and should be applied with corresponding patch for libosmocore.
- Use hex representation in VTY - Increased DB SCHEMA_REVISION - Implemented DB migration code
db_test is temporary broken because incremental migration isn't implemented yet (WIP).
Signed-off-by: Vadim Yanitskiy axilirator@gmail.com --- openbsc/include/openbsc/gsm_subscriber.h | 2 +- openbsc/src/libmsc/db.c | 155 +++++++++++++++++++++++++----- openbsc/src/libmsc/vty_interface_layer3.c | 3 +- openbsc/tests/gsm0408/gsm0408_test.c | 2 +- 4 files changed, 133 insertions(+), 29 deletions(-)
diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h index 7d6c776..785dc36 100644 --- a/openbsc/include/openbsc/gsm_subscriber.h +++ b/openbsc/include/openbsc/gsm_subscriber.h @@ -14,7 +14,7 @@
#define GSM_SUBSCRIBER_FIRST_CONTACT 0x00000001 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */ -#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
#define GSM_SUBSCRIBER_NO_EXPIRATION 0x0
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 0935fc5..04aee79 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -47,7 +47,7 @@ static char *db_basename = NULL; static char *db_dirname = NULL; static dbi_conn conn;
-#define SCHEMA_REVISION "4" +#define SCHEMA_REVISION "5"
enum { SCHEMA_META, @@ -83,7 +83,7 @@ static const char *create_stmts[] = { "name TEXT, " "extension TEXT UNIQUE, " "authorized INTEGER NOT NULL DEFAULT 0, " - "tmsi TEXT UNIQUE, " + "tmsi INTEGER UNIQUE, " "lac INTEGER NOT NULL DEFAULT 0, " "expire_lu TIMESTAMP DEFAULT NULL" ")", @@ -212,6 +212,7 @@ static int update_db_revision_2(void) } dbi_result_free(result);
+ LOGP(DDB, LOGL_NOTICE, "Migration complete.\n"); return 0; }
@@ -357,6 +358,7 @@ static int update_db_revision_3(void) else dbi_result_free(result);
+ LOGP(DDB, LOGL_NOTICE, "Migration complete.\n"); return 0;
rollback: @@ -369,6 +371,108 @@ rollback: return -EINVAL; }
+static int update_db_revision_4(void) +{ + dbi_result result; + + LOGP(DDB, LOGL_NOTICE, "Going to migrate from revision 4\n"); + + result = dbi_conn_query(conn, "BEGIN EXCLUSIVE TRANSACTION"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to begin transaction " + "(upgrade from rev 4)\n"); + return -EINVAL; + } + dbi_result_free(result); + + /* Rename old Subscriber table to be able create a new one */ + result = dbi_conn_query(conn, + "ALTER TABLE Subscriber RENAME TO Subscriber_4"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to rename the old Subscriber table " + "(upgrade from rev 4).\n"); + goto rollback; + } + dbi_result_free(result); + + /* Create new Subscriber table */ + result = dbi_conn_query(conn, create_stmts[SCHEMA_SUBSCRIBER]); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to create a new Subscriber table " + "(upgrade from rev 4).\n"); + goto rollback; + } + dbi_result_free(result); + + /* Copy subscriber data into the new table */ + result = dbi_conn_query(conn, + "INSERT INTO Subscriber " + "SELECT * FROM Subscriber_4"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to copy subscriber data into the new table " + "(upgrade from rev 4).\n"); + goto rollback; + } + dbi_result_free(result); + + /* Remove the temporary table */ + result = dbi_conn_query(conn, "DROP TABLE Subscriber_4"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to drop the old Subscriber table " + "(upgrade from rev 4).\n"); + goto rollback; + } + dbi_result_free(result); + + /* We're done. Bump DB Meta revision to 5 */ + result = dbi_conn_query(conn, + "UPDATE Meta " + "SET value = '5' " + "WHERE key = 'revision'"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to update DB schema revision " + "(upgrade from rev 4).\n"); + goto rollback; + } + dbi_result_free(result); + + result = dbi_conn_query(conn, "COMMIT TRANSACTION"); + if (!result) { + LOGP(DDB, LOGL_ERROR, + "Failed to commit the transaction " + "(upgrade from rev 4)\n"); + return -EINVAL; + } + + /* Shrink DB file size by actually wiping out Subscriber_4 table data */ + result = dbi_conn_query(conn, "VACUUM"); + if (!result) + LOGP(DDB, LOGL_ERROR, + "VACUUM failed. Ignoring it " + "(upgrade from rev 4).\n"); + else + dbi_result_free(result); + + LOGP(DDB, LOGL_NOTICE, "Migration complete.\n"); + return 0; + +rollback: + result = dbi_conn_query(conn, "ROLLBACK TRANSACTION"); + if (!result) + LOGP(DDB, LOGL_ERROR, + "Rollback failed " + "(upgrade from rev 4).\n"); + else + dbi_result_free(result); + return -EINVAL; +} + static int check_db_revision(void) { dbi_result result; @@ -388,20 +492,18 @@ static int check_db_revision(void) dbi_result_free(result); return -EINVAL; } - if (!strcmp(rev_s, "2")) { - if (update_db_revision_2()) { - 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, "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)) { + + if (!strcmp(rev_s, SCHEMA_REVISION)) { /* everything is fine */ + } else if (!strcmp(rev_s, "2")) { + if (update_db_revision_2()) + goto error; + } else if (!strcmp(rev_s, "3")) { + if (update_db_revision_3()) + goto error; + } else if (!strcmp(rev_s, "4")) { + if (update_db_revision_4()) + goto error; } else { LOGP(DDB, LOGL_FATAL, "Invalid database schema revision '%s'.\n", rev_s); dbi_result_free(result); @@ -410,6 +512,11 @@ static int check_db_revision(void)
dbi_result_free(result); return 0; + +error: + LOGP(DDB, LOGL_FATAL, "Failed to update database from schema revision '%s'.\n", rev_s); + dbi_result_free(result); + return -EINVAL; }
static int db_configure(void) @@ -808,10 +915,6 @@ static void db_set_from_query(struct gsm_subscriber *subscr, dbi_conn result) if (string) strncpy(subscr->imsi, string, GSM_IMSI_LENGTH-1);
- string = dbi_result_get_string(result, "tmsi"); - if (string) - subscr->tmsi = tmsi_from_string(string); - string = dbi_result_get_string(result, "name"); if (string) strncpy(subscr->name, string, GSM_NAME_LENGTH); @@ -820,7 +923,8 @@ static void db_set_from_query(struct gsm_subscriber *subscr, dbi_conn result) if (string) strncpy(subscr->extension, string, GSM_EXTENSION_LENGTH);
- subscr->lac = dbi_result_get_ulonglong(result, "lac"); + subscr->tmsi = dbi_result_get_ulonglong(result, "tmsi"); + subscr->lac = dbi_result_get_ulonglong(result, "lac");
if (!dbi_result_field_is_null(result, "expire_lu")) subscr->expire_lu = dbi_result_get_datetime(result, "expire_lu"); @@ -828,7 +932,6 @@ static void db_set_from_query(struct gsm_subscriber *subscr, dbi_conn result) subscr->expire_lu = GSM_SUBSCRIBER_NO_EXPIRATION;
subscr->authorized = dbi_result_get_ulonglong(result, "authorized"); - }
#define BASE_QUERY "SELECT * FROM Subscriber " @@ -893,9 +996,10 @@ struct gsm_subscriber *db_get_subscriber(enum gsm_subscriber_field field, subscr->id = dbi_result_get_ulonglong(result, "id");
db_set_from_query(subscr, result); - DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', TMSI %u, EXTEN '%s', LAC %hu, AUTH %u\n", - subscr->id, subscr->imsi, subscr->name, subscr->tmsi, subscr->extension, - subscr->lac, subscr->authorized); + DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', " + "TMSI 0x%08x, EXTEN '%s', LAC %hu, AUTH %u\n", + subscr->id, subscr->imsi, subscr->name, subscr->tmsi, + subscr->extension, subscr->lac, subscr->authorized); dbi_result_free(result);
get_equipment_by_subscr(subscr); @@ -998,7 +1102,6 @@ int db_sync_subscriber(struct gsm_subscriber *subscriber) }
dbi_result_free(result); - return 0; }
@@ -1225,7 +1328,7 @@ int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) } if (!dbi_result_next_row(result)) { dbi_result_free(result); - DEBUGP(DDB, "Allocated TMSI %u for IMSI %s.\n", + DEBUGP(DDB, "Allocated TMSI 0x%08x for IMSI %s.\n", subscriber->tmsi, subscriber->imsi); return db_sync_subscriber(subscriber); } diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index f49c53a..9879a53 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -189,7 +189,8 @@ static struct gsm_subscriber *get_subscr_by_argv(struct gsm_network *gsmnet, else if (!strcmp(type, "imsi")) return subscr_get_by_imsi(gsmnet->subscr_group, id); else if (!strcmp(type, "tmsi")) - return subscr_get_by_tmsi(gsmnet->subscr_group, atoi(id)); + return subscr_get_by_tmsi(gsmnet->subscr_group, + tmsi_from_string(id)); else if (!strcmp(type, "id")) return subscr_get_by_id(gsmnet->subscr_group, atoi(id));
diff --git a/openbsc/tests/gsm0408/gsm0408_test.c b/openbsc/tests/gsm0408/gsm0408_test.c index 781ef61..8ed57ca 100644 --- a/openbsc/tests/gsm0408/gsm0408_test.c +++ b/openbsc/tests/gsm0408/gsm0408_test.c @@ -93,7 +93,7 @@ static void test_mi_functionality(void) /* tmsi code */ mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi); gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2); - COMPARE((uint32_t)strtoul(mi_parsed, NULL, 10), ==, tmsi); + COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi);
/* imsi code */ mi_len = gsm48_generate_mid_from_imsi(mi, imsi_odd);
-
Holger Freyther -
Vadim Yanitskiy -
Вадим Яницкий