1 Apr
2016
1 Apr
'16
8:33 p.m.
From: Holger Hans Peter Freyther <holger(a)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
1 Apr
1 Apr
8:33 p.m.
New subject: [PATCH 2/3] subscr: Make db_create_subscriber fail on duplicates
From: Holger Hans Peter Freyther <holger(a)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(a)debian.org>
- * (C) 2009 by Holger Hans Peter Freyther <zecke(a)selfish.org>
+ * (C) 2009-2016 by Holger Hans Peter Freyther <zecke(a)selfish.org>
* (C) 2014 by Alexander Chemeris <Alexander.Chemeris(a)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
6 Apr
6 Apr
12:34 p.m.
New subject: [PATCH 2/3] subscr: Make db_create_subscriber fail on duplicates
On Fri, Apr 01, 2016 at 08:33:34PM +0200, Holger Freyther wrote:
From: Holger Hans Peter Freyther
<holger(a)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
1 Apr
1 Apr
8:33 p.m.
New subject: [PATCH 3/3] db: If creating a subscriber in the db fails, return NULL
From: Holger Hans Peter Freyther <holger(a)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
6 Apr
6 Apr
12:37 p.m.
New subject: [PATCH 3/3] db: If creating a subscriber in the db fails, return NULL
On Fri, Apr 01, 2016 at 08:33:35PM +0200, Holger Freyther wrote:
From: Holger Hans Peter Freyther
<holger(a)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
12:41 p.m.
New subject: [PATCH 3/3] db: If creating a subscriber in the db fails, return NULL
On 06 Apr 2016, at 12:37, Neels Hofmeyr
<nhofmeyr(a)sysmocom.de> wrote:
I don't know. My test tries to explore it but right now I think OS#1657 is not an
issue.
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?
12:24 p.m.
On Fri, Apr 01, 2016 at 08:33:33PM +0200, Holger Freyther wrote:
From: Holger Hans Peter Freyther
<holger(a)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
2963
days inactive
2968
days old
6 comments
2 participants
participants (2)
-
Holger Freyther -
Neels Hofmeyr