[PATCH] db: Add more tests for retrieving subscribers from a DB and avoid NULL dereference when something goes wrong in tests.
--- openbsc/tests/db/db_test.c | 95 ++++++++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 26 deletions(-)
diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c index c3beee2..a4259ab 100644 --- a/openbsc/tests/db/db_test.c +++ b/openbsc/tests/db/db_test.c @@ -26,39 +26,52 @@ #include <stdio.h> #include <string.h> #include <stdlib.h> +#include <inttypes.h>
static struct gsm_network dummy_net;
#define SUBSCR_PUT(sub) \ - sub->net = &dummy_net; \ - subscr_put(sub); + if (sub) { \ + sub->net = &dummy_net; \ + subscr_put(sub); \ + }
#define COMPARE(original, copy) \ - if (original->id != copy->id) \ - printf("Ids do not match in %s:%d %llu %llu\n", \ - __FUNCTION__, __LINE__, original->id, copy->id); \ - if (original->lac != copy->lac) \ - printf("LAC do not match in %s:%d %d %d\n", \ - __FUNCTION__, __LINE__, original->lac, copy->lac); \ - if (original->authorized != copy->authorized) \ - printf("Authorize do not match in %s:%d %d %d\n", \ - __FUNCTION__, __LINE__, original->authorized, \ - copy->authorized); \ - if (strcmp(original->imsi, copy->imsi) != 0) \ - printf("IMSIs do not match in %s:%d '%s' '%s'\n", \ - __FUNCTION__, __LINE__, original->imsi, copy->imsi); \ - if (original->tmsi != copy->tmsi) \ - printf("TMSIs do not match in %s:%d '%u' '%u'\n", \ - __FUNCTION__, __LINE__, original->tmsi, copy->tmsi); \ - if (strcmp(original->name, copy->name) != 0) \ - printf("names do not match in %s:%d '%s' '%s'\n", \ - __FUNCTION__, __LINE__, original->name, copy->name); \ - if (strcmp(original->extension, copy->extension) != 0) \ - printf("Extensions do not match in %s:%d '%s' '%s'\n", \ - __FUNCTION__, __LINE__, original->extension, copy->extension); \ + if (!original) \ + printf("NULL original in %s:%d\n", \ + __FUNCTION__, __LINE__); \ + if (!copy) \ + printf("NULL copy in %s:%d\n", \ + __FUNCTION__, __LINE__); \ + if (original && copy) { \ + if (original->id != copy->id) \ + printf("Ids do not match in %s:%d %llu %llu\n", \ + __FUNCTION__, __LINE__, original->id, copy->id); \ + if (original->lac != copy->lac) \ + printf("LAC do not match in %s:%d %d %d\n", \ + __FUNCTION__, __LINE__, original->lac, copy->lac); \ + if (original->authorized != copy->authorized) \ + printf("Authorize do not match in %s:%d %d %d\n", \ + __FUNCTION__, __LINE__, original->authorized, \ + copy->authorized); \ + if (strcmp(original->imsi, copy->imsi) != 0) \ + printf("IMSIs do not match in %s:%d '%s' '%s'\n", \ + __FUNCTION__, __LINE__, original->imsi, copy->imsi); \ + if (original->tmsi != copy->tmsi) \ + printf("TMSIs do not match in %s:%d '%u' '%u'\n", \ + __FUNCTION__, __LINE__, original->tmsi, copy->tmsi); \ + if (strcmp(original->name, copy->name) != 0) \ + printf("names do not match in %s:%d '%s' '%s'\n", \ + __FUNCTION__, __LINE__, original->name, copy->name); \ + if (strcmp(original->extension, copy->extension) != 0) \ + printf("Extensions do not match in %s:%d '%s' '%s'\n", \ + __FUNCTION__, __LINE__, original->extension, copy->extension); \ + }
int main() { + char scratch_str[256]; + printf("Testing subscriber database code.\n"); osmo_init_logging(&log_info);
@@ -91,10 +104,25 @@ int main() db_subscriber_alloc_tmsi(alice); alice->lac=42; db_sync_subscriber(alice); + /* Get by TMSI */ + snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_TMSI, scratch_str); + COMPARE(alice, alice_db); + SUBSCR_PUT(alice_db); + /* Get by IMSI */ alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_IMSI, alice_imsi); COMPARE(alice, alice_db); - SUBSCR_PUT(alice); SUBSCR_PUT(alice_db); + /* Get by id */ + snprintf(scratch_str, sizeof(scratch_str), "%llu", alice->id); + alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_ID, scratch_str); + COMPARE(alice, alice_db); + SUBSCR_PUT(alice_db); + /* Get by extension */ + alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_EXTENSION, alice->extension); + COMPARE(alice, alice_db); + SUBSCR_PUT(alice_db); + SUBSCR_PUT(alice);
alice_imsi = "9993245423445"; alice = db_create_subscriber(NULL, alice_imsi); @@ -103,10 +131,25 @@ int main() db_sync_subscriber(alice); db_subscriber_assoc_imei(alice, "1234567890"); db_subscriber_assoc_imei(alice, "6543560920"); + /* Get by TMSI */ + snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_TMSI, scratch_str); + COMPARE(alice, alice_db); + SUBSCR_PUT(alice_db); + /* Get by IMSI */ alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_IMSI, alice_imsi); COMPARE(alice, alice_db); - SUBSCR_PUT(alice); SUBSCR_PUT(alice_db); + /* Get by id */ + snprintf(scratch_str, sizeof(scratch_str), "%llu", alice->id); + alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_ID, scratch_str); + COMPARE(alice, alice_db); + SUBSCR_PUT(alice_db); + /* Get by extension */ + alice_db = db_get_subscriber(NULL, GSM_SUBSCRIBER_EXTENSION, alice->extension); + COMPARE(alice, alice_db); + SUBSCR_PUT(alice_db); + SUBSCR_PUT(alice);
db_fini();
On Sat, Oct 05, 2013 at 12:16:07PM +0200, Alexander Chemeris wrote:
#define SUBSCR_PUT(sub) \
- sub->net = &dummy_net; \
- subscr_put(sub);
- if (sub) { \
sub->net = &dummy_net; \subscr_put(sub); \- }
I don't like this semantic change. Either the tests expects a valid subscriber or it doesn't. It is not a "maybe" this query will result in a look up.
On Sun, Oct 6, 2013 at 1:41 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, Oct 05, 2013 at 12:16:07PM +0200, Alexander Chemeris wrote:
#define SUBSCR_PUT(sub) \
sub->net = &dummy_net; \subscr_put(sub);
if (sub) { \sub->net = &dummy_net; \subscr_put(sub); \}I don't like this semantic change. Either the tests expects a valid subscriber or it doesn't. It is not a "maybe" this query will result in a look up.
My changes are to avoid _segfault_ when something goes wrong. Test will still fail, because it will print info about NULL original or copy, and comparison with the proper output will fail.
I decided to do it that way, because it makes the code of the test itself looking clean. Otherwise we have to put this "if(sub)" to every test, which looks ugly. But if you prefer some other way - please go ahead, I don't really care about this. I just tried to keep the clean look.
-
Alexander Chemeris -
Holger Hans Peter Freyther