[PATCH 1/3] subscr: Add testcase creating an already created subscriber
From: Holger Hans Peter Freyther <holger@moiji-mobile.com> Add testcase to issue the subscriber create twice. db_create_subscriber in db.c will first try to find the subscriber and if it exists, it will update the "updated" column in the database. Related: OS Issue #1657 --- openbsc/tests/vty_test_runner.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index 8db0825..620254e 100644 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -307,6 +307,34 @@ class TestVTYNITB(TestVTYGenericBSC): if classNum != 10: self.assertEquals(res.find("rach access-control-class " + str(classNum) + " barred"), -1) + def testSubscriberCreateDeleteTwice(self): + self.vty.enable() + + imsi = "204300854013739" + + # Initially we don't have this subscriber + self.vty.verify('show subscriber imsi '+imsi, ['% No subscriber found for imsi '+imsi]) + + # Lets create one + res = self.vty.command('subscriber create imsi '+imsi) + self.assert_(res.find(" IMSI: "+imsi) > 0) + res2 = self.vty.command('subscriber create imsi '+imsi) + self.assert_(res2.find(" IMSI: "+imsi) > 0) + self.assertEqual(res, res2) + + # Now we have it + res = self.vty.command('show subscriber imsi '+imsi) + self.assert_(res.find(" IMSI: "+imsi) > 0) + + # Delete it + res = self.vty.command('subscriber delete imsi '+imsi) + self.assert_(res != "") + + # Now it should not be there anymore + res = self.vty.command('show subscriber imsi '+imsi) + self.assert_(res != '% No subscriber found for imsi '+imsi) + + def testSubscriberCreateDelete(self): self.vty.enable() -- 2.6.3
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(); -- 2.6.3
On Fri, Apr 01, 2016 at 08:33:34PM +0200, Holger Freyther wrote:
From: Holger Hans Peter Freyther <holger@moiji-mobile.com>
The issue of db_create_subscriber updating an already existing subcr
s/subcr/subscr ;)
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 do not update the 'updated' timestamp of an already existing subscriber. Add a testcase for this behavior. ~Neels
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); -- 2.6.3
On Fri, Apr 01, 2016 at 08:33:35PM +0200, Holger Freyther wrote:
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
curious: 1657 says "VTY command seems to be able to crash OsmoNITB", how did that happen, and does this patch set change anything about it? ~Neels
On 06 Apr 2016, at 12:37, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
Related: OS Issue #1657
curious: 1657 says "VTY command seems to be able to crash OsmoNITB", how did that happen, and does this patch set change anything about it?
I don't know. My test tries to explore it but right now I think OS#1657 is not an issue.
On Fri, Apr 01, 2016 at 08:33:33PM +0200, Holger Freyther wrote:
From: Holger Hans Peter Freyther <holger@moiji-mobile.com>
Add testcase to issue the subscriber create twice.
Could be nice to add this clarification in the test code as comment:
db_create_subscriber in db.c will first try to find the subscriber and if it exists, it will update the "updated" column in the database.
[...]
+ + # Lets create one + res = self.vty.command('subscriber create imsi '+imsi) + self.assert_(res.find(" IMSI: "+imsi) > 0)
... right here.
+ res2 = self.vty.command('subscriber create imsi '+imsi) + self.assert_(res2.find(" IMSI: "+imsi) > 0) + self.assertEqual(res, res2)
The test isn't testing the update of the 'updated' column, right? Why though would the db want to update the 'updated' column for such a noop anyway? ~Neels
participants (2)
-
Holger Freyther -
Neels Hofmeyr