[PATCH 0/6] Four minor fixes, "subscriber create" and subscriber's info formatting.
This patch set is mostly minor fixes and clean ups, with the exception of the last two patches ("subscriber create" VTY command and a standard way for printing a subscriber's info). I would appreciate merging this patches to master, as I'm working onmore code which depends on them. Alexander Chemeris (6): Fix copy-paste error in console output in db_test. Fix typo ',' -> ';' at the end of a line. Fix typo in console output: "PEROIDOC" -> "PERIODIC". Slight clean up of the code in gsm340_rx_tpdu() and a fix for an unlikely, but possible memory leak there. Add "subscriber create" VTY command. Introduce a standard way for printing a subscriber's info. openbsc/include/openbsc/gsm_subscriber.h | 4 ++++ openbsc/src/libmsc/db.c | 2 +- openbsc/src/libmsc/gsm_04_08.c | 2 +- openbsc/src/libmsc/gsm_04_11.c | 7 ++++--- openbsc/src/libmsc/vty_interface_layer3.c | 27 +++++++++++++++++++++++++++ openbsc/tests/db/db_test.c | 2 +- 6 files changed, 38 insertions(+), 6 deletions(-) -- 1.7.9.5
--- openbsc/tests/db/db_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c index c0ee618..c3beee2 100644 --- a/openbsc/tests/db/db_test.c +++ b/openbsc/tests/db/db_test.c @@ -54,7 +54,7 @@ static struct gsm_network dummy_net; 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("names do not match in %s:%d '%s' '%s'\n", \ + printf("Extensions do not match in %s:%d '%s' '%s'\n", \ __FUNCTION__, __LINE__, original->extension, copy->extension); \ int main() -- 1.7.9.5
Funny, this is a correct C expression and doesn't change execution, thus it stayed unnoticed for quite a while. --- openbsc/src/libmsc/db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 4e20e23..12d60bb 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -996,7 +996,7 @@ int db_subscriber_assoc_imei(struct gsm_subscriber *subscriber, char imei[GSM_IM dbi_result result; strncpy(subscriber->equipment.imei, imei, - sizeof(subscriber->equipment.imei)-1), + sizeof(subscriber->equipment.imei)-1); result = dbi_conn_queryf(conn, "INSERT OR IGNORE INTO Equipment " -- 1.7.9.5
--- openbsc/src/libmsc/gsm_04_08.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index cd31f69..f771b02 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -513,7 +513,7 @@ static const char *lupd_name(uint8_t type) case GSM48_LUPD_NORMAL: return "NORMAL"; case GSM48_LUPD_PERIODIC: - return "PEROIDOC"; + return "PERIODIC"; case GSM48_LUPD_IMSI_ATT: return "IMSI ATTACH"; default: -- 1.7.9.5
--- openbsc/src/libmsc/gsm_04_11.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openbsc/src/libmsc/gsm_04_11.c b/openbsc/src/libmsc/gsm_04_11.c index 2fc250b..e554b74 100644 --- a/openbsc/src/libmsc/gsm_04_11.c +++ b/openbsc/src/libmsc/gsm_04_11.c @@ -337,8 +337,8 @@ static int gsm340_rx_tpdu(struct gsm_subscriber_connection *conn, struct msgb *m sms_alphabet = gsm338_get_sms_alphabet(gsms->data_coding_scheme); if (sms_alphabet == 0xffffffff) { - sms_free(gsms); - return GSM411_RP_CAUSE_MO_NET_OUT_OF_ORDER; + rc = GSM411_RP_CAUSE_MO_NET_OUT_OF_ORDER; + goto out; } switch (sms_vpf) { @@ -359,7 +359,8 @@ static int gsm340_rx_tpdu(struct gsm_subscriber_connection *conn, struct msgb *m default: LOGP(DLSMS, LOGL_NOTICE, "SMS Validity period not implemented: 0x%02x\n", sms_vpf); - return GSM411_RP_CAUSE_MO_NET_OUT_OF_ORDER; + rc = GSM411_RP_CAUSE_MO_NET_OUT_OF_ORDER; + goto out; } gsms->user_data_len = *smsp++; if (gsms->user_data_len) { -- 1.7.9.5
It may be useful in production, but it's really required for VTY testing of subscriber related commands. --- openbsc/src/libmsc/vty_interface_layer3.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index 8dc2812..dd657fb 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -210,6 +210,32 @@ DEFUN(show_subscr, return CMD_SUCCESS; } +DEFUN(subscriber_create, + subscriber_create_cmd, + "subscriber create imsi ID", + "Operations on a Subscriber\n" \ + "Create new subscriber\n" \ + "Identify the subscriber by his IMSI\n" \ + "Identifier for the subscriber\n") +{ + struct gsm_network *gsmnet = gsmnet_from_vty(vty); + struct gsm_subscriber *subscr; + + subscr = db_create_subscriber(gsmnet, 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. */ + subscr_dump_full_vty(vty, subscr, 0); + + subscr_put(subscr); + + return CMD_SUCCESS; +} + DEFUN(subscriber_send_pending_sms, subscriber_send_pending_sms_cmd, "subscriber " SUBSCR_TYPES " ID sms pending-send", @@ -934,6 +960,7 @@ int bsc_vty_init_extra(void) install_element_ve(&sms_send_pend_cmd); + install_element_ve(&subscriber_create_cmd); install_element_ve(&subscriber_send_sms_cmd); install_element_ve(&subscriber_silent_sms_cmd); install_element_ve(&subscriber_silent_call_start_cmd); -- 1.7.9.5
On Fri, Oct 04, 2013 at 02:42:28AM +0200, Alexander Chemeris wrote:
It may be useful in production, but it's really required for VTY testing of subscriber related commands.
Hi, nice patch. Please create a VTY unit test for this. holger
On Fri, Oct 4, 2013 at 2:22 AM, Holger Hans Peter Freyther <holger@freyther.de> wrote:
On Fri, Oct 04, 2013 at 02:42:28AM +0200, Alexander Chemeris wrote:
It may be useful in production, but it's really required for VTY testing of subscriber related commands.
nice patch. Please create a VTY unit test for this.
Yeah, I was thinking about that, but haven't come up with a good set of things to do. What would you propose to test in the VTY test here? The only thing I could think of is issuing "subscriber create" and then "show subscriber" and checking that they show the same. Btw, what do you think about having "subscriber show" command as a alias for "show subscriber" command? -- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
On Fri, Oct 04, 2013 at 06:34:31AM -0400, Alexander Chemeris wrote:
Yeah, I was thinking about that, but haven't come up with a good set of things to do. What would you propose to test in the VTY test here? The only thing I could think of is issuing "subscriber create" and then "show subscriber" and checking that they show the same.
yes, that is all. show subscriber imsi 12345.. see that it fails, create the subscriber, do the show again and see it is working.
On Fri, Oct 4, 2013 at 1:25 PM, Holger Hans Peter Freyther <holger@freyther.de> wrote:
On Fri, Oct 04, 2013 at 06:34:31AM -0400, Alexander Chemeris wrote:
Yeah, I was thinking about that, but haven't come up with a good set of things to do. What would you propose to test in the VTY test here? The only thing I could think of is issuing "subscriber create" and then "show subscriber" and checking that they show the same.
yes, that is all. show subscriber imsi 12345.. see that it fails, create the subscriber, do the show again and see it is working.
Ok, sounds good, I'll do that. -- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
The intention is to standardize subscriber identification output in log files. At this moment we sometimes identify subscriber by id, sometimes by extension, sometimes by IMSI and sometimes do not idenify at all. This makes log analysis hard or impossible. This patch doesn't touch actual log output, as it's a separate thing and requires consideration before checking in. --- openbsc/include/openbsc/gsm_subscriber.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h index 78f9710..f21b3e6 100644 --- a/openbsc/include/openbsc/gsm_subscriber.h +++ b/openbsc/include/openbsc/gsm_subscriber.h @@ -15,6 +15,10 @@ #define GSM_SUBSCRIBER_FIRST_CONTACT 0x00000001 #define tmsi_from_string(str) strtoul(str, NULL, 10) +#define GSM_SUBS_FMT_STR "IMSI %s (id %llu, ext %s%s%s)" +#define GSM_SUBS_FMT_VAL(x) (x)->imsi, (x)->id, (x)->extension, \ + strlen((x)->name)?", name ":"", strlen((x)->name)?(x)->name:"" + struct vty; struct gsm_equipment { -- 1.7.9.5
On Fri, Oct 04, 2013 at 02:42:29AM +0200, Alexander Chemeris wrote:
This patch doesn't touch actual log output, as it's a separate thing and requires consideration before checking in.
I will merge this once there is an user.
On Fri, Oct 4, 2013 at 8:17 AM, Holger Hans Peter Freyther <holger@freyther.de> wrote:
On Fri, Oct 04, 2013 at 02:42:29AM +0200, Alexander Chemeris wrote:
This patch doesn't touch actual log output, as it's a separate thing and requires consideration before checking in.
I will merge this once there is an user.
Ok, I'll re-submit this as a part of another series of patches. I wanted to get some review before I start using this widely. -- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
Hi, Two cents: On Fri, Oct 4, 2013 at 3:42 AM, Alexander Chemeris <alexander.chemeris@gmail.com> wrote:
+#define GSM_SUBS_FMT_STR "IMSI %s (id %llu, ext %s%s%s)" +#define GSM_SUBS_FMT_VAL(x) (x)->imsi, (x)->id, (x)->extension, \ + strlen((x)->name)?", name ":"", strlen((x)->name)?(x)->name:""
Checking for an empty string might be done by testing *s or s[0] instead of strlen(s). Also the second check for an empty string is superfluous, since it'll print the same either way. So one might write that as: #define GSM_SUBS_FMT_VAL(x) (x)->imsi, (x)->id, (x)->extension, \ (x)->name[0] ? ", name " : "", (x)->name Cheers, Alex
Alex, On Fri, Oct 4, 2013 at 8:58 AM, Alex Badea <vamposdecampos@gmail.com> wrote:
On Fri, Oct 4, 2013 at 3:42 AM, Alexander Chemeris <alexander.chemeris@gmail.com> wrote:
+#define GSM_SUBS_FMT_STR "IMSI %s (id %llu, ext %s%s%s)" +#define GSM_SUBS_FMT_VAL(x) (x)->imsi, (x)->id, (x)->extension, \ + strlen((x)->name)?", name ":"", strlen((x)->name)?(x)->name:""
Checking for an empty string might be done by testing *s or s[0] instead of strlen(s).
I was thinking about this and decided for strlen(), because it's more human readable. But on the second look I think s[0] is not too bad. Thank you for bringing this up.
Also the second check for an empty string is superfluous, since it'll print the same either way.
Good catch, thanks.
So one might write that as:
#define GSM_SUBS_FMT_VAL(x) (x)->imsi, (x)->id, (x)->extension, \ (x)->name[0] ? ", name " : "", (x)->name
I'll use this for the second iteration of the patch. -- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
participants (3)
-
Alex Badea -
Alexander Chemeris -
Holger Hans Peter Freyther