From: Holger Hans Peter Freyther holger@moiji-mobile.com

The issue of db_create_subscriber updating an already existing subcr is that the same subscriber will then have two entries in the active subscribers list. In general this will break assumptions that a subscr can be compared by comparing the pointer.

In the case of the VTY this was not an issue as the created subscr was immediately destroyed again but it is better to avoid this problem.

Change the VTY command to find the subscriber and then call sync to have the updated time set. The side-effect is we will now have two queries for the subscriber. Once through subscr_get_by_imsi and once through db_create_subscriber.

Change the db_create_subscriber to fail if a subscriber already exists, and add a testcase for this behavior.

Related: OS Issue #1657 --- openbsc/src/libmsc/db.c | 10 ++-------- openbsc/src/libmsc/vty_interface_layer3.c | 16 +++++++++++----- openbsc/tests/db/db_test.c | 6 +++++- 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 0935fc5..17bea24 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -508,14 +508,8 @@ struct gsm_subscriber *db_create_subscriber(const char *imsi) /* Is this subscriber known in the db? */ subscr = db_get_subscriber(GSM_SUBSCRIBER_IMSI, imsi); if (subscr) { - result = dbi_conn_queryf(conn, - "UPDATE Subscriber set updated = datetime('now') " - "WHERE imsi = %s " , imsi); - if (!result) - LOGP(DDB, LOGL_ERROR, "failed to update timestamp\n"); - else - dbi_result_free(result); - return subscr; + subscr_put(subscr); + return NULL; }

subscr = subscr_alloc(); diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index f49c53a..790fedf 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -236,11 +236,17 @@ DEFUN(subscriber_create, struct gsm_network *gsmnet = gsmnet_from_vty(vty); struct gsm_subscriber *subscr;

- subscr = subscr_create_subscriber(gsmnet->subscr_group, argv[0]); - if (!subscr) { - vty_out(vty, "%% No subscriber created for IMSI %s%s", - argv[0], VTY_NEWLINE); - return CMD_WARNING; + subscr = subscr_get_by_imsi(gsmnet->subscr_group, argv[0]); + if (subscr) + db_sync_subscriber(subscr); + else { + subscr = subscr_create_subscriber(gsmnet->subscr_group, argv[0]); + + if (!subscr) { + vty_out(vty, "%% No subscriber created for IMSI %s%s", + argv[0], VTY_NEWLINE); + return CMD_WARNING; + } }

/* Show info about the created subscriber. */ diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c index a02d1f8..fb159a5 100644 --- a/openbsc/tests/db/db_test.c +++ b/openbsc/tests/db/db_test.c @@ -1,5 +1,5 @@ /* (C) 2008 by Jan Luebbe jluebbe@debian.org - * (C) 2009 by Holger Hans Peter Freyther zecke@selfish.org + * (C) 2009-2016 by Holger Hans Peter Freyther zecke@selfish.org * (C) 2014 by Alexander Chemeris Alexander.Chemeris@fairwaves.co * All Rights Reserved * @@ -246,6 +246,10 @@ int main() SUBSCR_PUT(alice_db); SUBSCR_PUT(alice);

+ /* create it again and see it fails */ + alice = db_create_subscriber(alice_imsi); + OSMO_ASSERT(!alice); + test_sms(); test_sms_migrate();

From: Holger Hans Peter Freyther holger@moiji-mobile.com

We should not return a subscriber in case it was not written to the database. Instead free the memory allocated and return NULL. Callers in gsm_04_08.c are prepared to have the creation fail.

Related: OS Issue #1657 --- openbsc/src/libmsc/db.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 17bea24..267b5ef 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -523,8 +523,11 @@ struct gsm_subscriber *db_create_subscriber(const char *imsi) "(%s, datetime('now'), datetime('now')) ", imsi ); - if (!result) + if (!result) { LOGP(DDB, LOGL_ERROR, "Failed to create Subscriber by IMSI.\n"); + subscr_put(subscr); + return NULL; + } subscr->id = dbi_conn_sequence_last(conn, NULL); strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH-1); dbi_result_free(result);