[PATCH 1/4] ussd: Send USSD on call setup on MSC errors
Send an USSD message to the mobile station requesting a connection for a call or a SMS when the link to the MSC is down or in the grace period. The messages can be set (and this feature activated) by setting bsc/missing-msc-text resp. msc/bsc-grace-text via the vty. The generation of both messages has been tested manually. Ticket: OW#957 --- openbsc/include/openbsc/osmo_bsc.h | 11 ++++- openbsc/include/openbsc/osmo_msc_data.h | 6 +++ openbsc/src/osmo-bsc/osmo_bsc_api.c | 58 ++++++++++++++++++++++++- openbsc/src/osmo-bsc/osmo_bsc_sccp.c | 16 ++++--- openbsc/src/osmo-bsc/osmo_bsc_vty.c | 71 +++++++++++++++++++++++++++++++ openbsc/tests/vty_test_runner.py | 35 ++++++++++++++- 6 files changed, 185 insertions(+), 12 deletions(-) diff --git a/openbsc/include/openbsc/osmo_bsc.h b/openbsc/include/openbsc/osmo_bsc.h index 1d216ac..1032daa 100644 --- a/openbsc/include/openbsc/osmo_bsc.h +++ b/openbsc/include/openbsc/osmo_bsc.h @@ -7,6 +7,13 @@ #define BSS_SEND_USSD 1 +enum bsc_con { + BSC_CON_SUCCESS, + BSC_CON_REJECT_NO_LINK, + BSC_CON_REJECT_RF_GRACE, + BSC_CON_NO_MEM, +}; + struct sccp_connection; struct osmo_msc_data; struct bsc_msc_connection; @@ -34,8 +41,8 @@ struct bsc_api *osmo_bsc_api(); int bsc_queue_for_msc(struct osmo_bsc_sccp_con *conn, struct msgb *msg); int bsc_open_connection(struct osmo_bsc_sccp_con *sccp, struct msgb *msg); -int bsc_create_new_connection(struct gsm_subscriber_connection *conn, - struct osmo_msc_data *msc); +enum bsc_con bsc_create_new_connection(struct gsm_subscriber_connection *conn, + struct osmo_msc_data *msc); int bsc_delete_connection(struct osmo_bsc_sccp_con *sccp); struct osmo_msc_data *bsc_find_msc(struct gsm_subscriber_connection *conn, struct msgb *); diff --git a/openbsc/include/openbsc/osmo_msc_data.h b/openbsc/include/openbsc/osmo_msc_data.h index 86b4a84..9c312ca 100644 --- a/openbsc/include/openbsc/osmo_msc_data.h +++ b/openbsc/include/openbsc/osmo_msc_data.h @@ -86,6 +86,9 @@ struct osmo_msc_data { /* ussd msc connection lost text */ char *ussd_msc_lost_txt; + + /* ussd text when MSC has entered the grace period */ + char *ussd_grace_txt; }; /* @@ -103,6 +106,9 @@ struct osmo_bsc_data { char *rf_ctrl_name; struct osmo_bsc_rf *rf_ctrl; int auto_off_timeout; + + /* ussd text when there is no MSC available */ + char *ussd_no_msc_txt; }; diff --git a/openbsc/src/osmo-bsc/osmo_bsc_api.c b/openbsc/src/osmo-bsc/osmo_bsc_api.c index df8c044..5eb3de6 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_api.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_api.c @@ -85,6 +85,48 @@ static void bsc_cipher_mode_compl(struct gsm_subscriber_connection *conn, queue_msg_or_return(resp); } +static void bsc_send_ussd_notification(struct gsm_subscriber_connection *conn, + struct msgb *msg, const char *text) +{ + struct gsm48_hdr *gh; + int8_t pdisc; + uint8_t mtype; + int drop_message = 1; + + if (! text) + return; + + if (! msg || msgb_l3len(msg) < sizeof(*gh)) { + return; + } + + gh = msgb_l3(msg); + pdisc = gh->proto_discr & 0x0f; + mtype = gh->msg_type & 0xbf; + + /* Is CM service request? */ + if (pdisc == GSM48_PDISC_MM && mtype == GSM48_MT_MM_CM_SERV_REQ) { + struct gsm48_service_request *cm; + + cm = (struct gsm48_service_request *) &gh->data[0]; + + /* Is type SMS or call? */ + if (cm->cm_service_type == GSM48_CMSERV_SMS) + drop_message = 0; + else if (cm->cm_service_type == GSM48_CMSERV_MO_CALL_PACKET) + drop_message = 0; + } + + if (drop_message) { + LOGP(DMSC, LOGL_DEBUG, "Skipping (not sending) USSD message: '%s'\n", text); + return; + } + + LOGP(DMSC, LOGL_INFO, "Sending USSD message: '%s'\n", text); + gsm0480_send_ussdNotify(conn, 1, text); + gsm0480_send_releaseComplete(conn); +} + /* * Instruct to reserve data for a new connectiom, create the complete * layer three message, send it to open the connection. @@ -100,6 +142,7 @@ static int bsc_compl_l3(struct gsm_subscriber_connection *conn, struct msgb *msg msc = bsc_find_msc(conn, msg); if (!msc) { LOGP(DMSC, LOGL_ERROR, "Failed to find a MSC for a connection.\n"); + bsc_send_ussd_notification(conn, msg, conn->bts->network->bsc_data->ussd_no_msc_txt); return -1; } @@ -112,10 +155,23 @@ static int complete_layer3(struct gsm_subscriber_connection *conn, struct msgb *resp; uint16_t network_code; uint16_t country_code; + enum bsc_con ret; /* allocate resource for a new connection */ - if (bsc_create_new_connection(conn, msc) != 0) + ret = bsc_create_new_connection(conn, msc); + + if (ret != BSC_CON_SUCCESS) { + /* allocation has failed */ + if (ret == BSC_CON_REJECT_NO_LINK) { + bsc_send_ussd_notification(conn, msg, msc->ussd_msc_lost_txt); + } else if (ret == BSC_CON_REJECT_RF_GRACE) { + bsc_send_ussd_notification(conn, msg, msc->ussd_grace_txt); + } + return BSC_API_CONN_POL_REJECT; + } + + // check return value, if failed check msg for and send USSD network_code = get_network_code_for_msc(conn->sccp_con->msc); country_code = get_country_code_for_msc(conn->sccp_con->msc); diff --git a/openbsc/src/osmo-bsc/osmo_bsc_sccp.c b/openbsc/src/osmo-bsc/osmo_bsc_sccp.c index 87f415e..deda0be 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_sccp.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_sccp.c @@ -188,35 +188,37 @@ int bsc_queue_for_msc(struct osmo_bsc_sccp_con *conn, struct msgb *msg) return 0; } -int bsc_create_new_connection(struct gsm_subscriber_connection *conn, +enum bsc_con bsc_create_new_connection(struct gsm_subscriber_connection *conn, struct osmo_msc_data *msc) { struct osmo_bsc_sccp_con *bsc_con; struct sccp_connection *sccp; /* This should not trigger */ - if (!msc->msc_con->is_authenticated) { + if (!msc || !msc->msc_con->is_authenticated) { + // VSAT link down LOGP(DMSC, LOGL_ERROR, "How did this happen? MSC is not connected. Dropping.\n"); - return -1; + return BSC_CON_REJECT_NO_LINK; } if (!bsc_grace_allow_new_connection(conn->bts->network, conn->bts)) { + // Approaching shore LOGP(DMSC, LOGL_NOTICE, "BSC in grace period. No new connections.\n"); - return -1; + return BSC_CON_REJECT_RF_GRACE; } sccp = sccp_connection_socket(); if (!sccp) { LOGP(DMSC, LOGL_ERROR, "Failed to allocate memory.\n"); - return -ENOMEM; + return BSC_CON_NO_MEM; } bsc_con = talloc_zero(conn->bts, struct osmo_bsc_sccp_con); if (!bsc_con) { LOGP(DMSC, LOGL_ERROR, "Failed to allocate.\n"); sccp_connection_free(sccp); - return -1; + return BSC_CON_NO_MEM; } /* callbacks */ @@ -237,7 +239,7 @@ int bsc_create_new_connection(struct gsm_subscriber_connection *conn, bsc_con->conn = conn; llist_add_tail(&bsc_con->entry, &active_connections); conn->sccp_con = bsc_con; - return 0; + return BSC_CON_SUCCESS; } int bsc_open_connection(struct osmo_bsc_sccp_con *conn, struct msgb *msg) diff --git a/openbsc/src/osmo-bsc/osmo_bsc_vty.c b/openbsc/src/osmo-bsc/osmo_bsc_vty.c index 90b0a0c..6f7cdbf 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_vty.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_vty.c @@ -122,6 +122,11 @@ static void write_msc(struct vty *vty, struct osmo_msc_data *msc) else vty_out(vty, " no bsc-msc-lost-text%s", VTY_NEWLINE); + if (msc->ussd_grace_txt && msc->ussd_grace_txt[0]) + vty_out(vty, " bsc-grace-text %s%s", msc->ussd_grace_txt, VTY_NEWLINE); + else + vty_out(vty, " no bsc-grace-text%s", VTY_NEWLINE); + if (msc->audio_length != 0) { int i; @@ -182,6 +187,11 @@ static int config_write_bsc(struct vty *vty) vty_out(vty, " bsc-auto-rf-off %d%s", bsc->auto_off_timeout, VTY_NEWLINE); + if (bsc->ussd_no_msc_txt && bsc->ussd_no_msc_txt[0]) + vty_out(vty, " missing-msc-text %s%s", bsc->ussd_no_msc_txt, VTY_NEWLINE); + else + vty_out(vty, " no missing-msc-text%s", VTY_NEWLINE); + return CMD_SUCCESS; } @@ -417,6 +427,63 @@ DEFUN(cfg_net_msc_no_lost_ussd, return CMD_SUCCESS; } +DEFUN(cfg_net_msc_grace_ussd, + cfg_net_msc_grace_ussd_cmd, + "bsc-grace-text .TEXT", + "Set the USSD notification to be sent when the MSC has entered the grace period\n" "Text to be sent\n") +{ + struct osmo_msc_data *data = osmo_msc_data(vty); + char *str = argv_concat(argv, argc, 0); + if (!str) + return CMD_WARNING; + + bsc_replace_string(osmo_bsc_data(vty), &data->ussd_grace_txt, str); + talloc_free(str); + return CMD_SUCCESS; +} + +DEFUN(cfg_net_msc_no_grace_ussd, + cfg_net_msc_no_grace_ussd_cmd, + "no bsc-grace-text", + NO_STR "Clear the USSD notification to be sent when the MSC has entered the grace period\n") +{ + struct osmo_msc_data *data = osmo_msc_data(vty); + + talloc_free(data->ussd_grace_txt); + data->ussd_grace_txt = 0; + + return CMD_SUCCESS; +} + +DEFUN(cfg_net_bsc_missing_msc_ussd, + cfg_net_bsc_missing_msc_ussd_cmd, + "missing-msc-text .TEXT", + "Set the USSD notification to be send when a MSC has not been found.\n" "Text to be sent\n") +{ + struct osmo_bsc_data *data = osmo_bsc_data(vty); + char *txt = argv_concat(argv, argc, 0); + if (!txt) + return CMD_WARNING; + + bsc_replace_string(data, &data->ussd_no_msc_txt, txt); + talloc_free(txt); + return CMD_SUCCESS; +} + +DEFUN(cfg_net_bsc_no_missing_msc_text, + cfg_net_bsc_no_missing_msc_text_cmd, + "no missing-msc-text", + NO_STR "Clear the USSD notification to be send when a MSC has not been found.\n") +{ + struct osmo_bsc_data *data = osmo_bsc_data(vty); + + talloc_free(data->ussd_no_msc_txt); + data->ussd_no_msc_txt = 0; + + return CMD_SUCCESS; +} + + DEFUN(cfg_net_msc_type, cfg_net_msc_type_cmd, "type (normal|local)", @@ -615,6 +682,8 @@ int bsc_vty_init_extra(void) install_element(BSC_NODE, &cfg_net_rf_socket_cmd); install_element(BSC_NODE, &cfg_net_rf_off_time_cmd); install_element(BSC_NODE, &cfg_net_no_rf_off_time_cmd); + install_element(BSC_NODE, &cfg_net_bsc_missing_msc_ussd_cmd); + install_element(BSC_NODE, &cfg_net_bsc_no_missing_msc_text_cmd); install_node(&msc_node, config_write_msc); bsc_install_default(MSC_NODE); @@ -631,6 +700,8 @@ int bsc_vty_init_extra(void) install_element(MSC_NODE, &cfg_net_msc_no_welcome_ussd_cmd); install_element(MSC_NODE, &cfg_net_msc_lost_ussd_cmd); install_element(MSC_NODE, &cfg_net_msc_no_lost_ussd_cmd); + install_element(MSC_NODE, &cfg_net_msc_grace_ussd_cmd); + install_element(MSC_NODE, &cfg_net_msc_no_grace_ussd_cmd); install_element(MSC_NODE, &cfg_net_msc_type_cmd); install_element(MSC_NODE, &cfg_net_msc_emerg_cmd); install_element(MSC_NODE, &cfg_net_msc_local_prefix_cmd); diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index ab9670c..460b70e 100644 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -207,7 +207,7 @@ class TestVTYBSC(TestVTYGenericBSC): self.vty.command("msc 0") self.assertEquals(self.vty.node(), 'config-msc') - def testUssdNotifications(self): + def testUssdNotificationsMsc(self): self.vty.enable() self.vty.command("configure terminal") self.vty.command("msc") @@ -215,10 +215,12 @@ class TestVTYBSC(TestVTYGenericBSC): # Test invalid input self.vty.verify("bsc-msc-lost-text", ['% Command incomplete.']) self.vty.verify("bsc-welcome-text", ['% Command incomplete.']) + self.vty.verify("bsc-grace-text", ['% Command incomplete.']) # Enable USSD notifications self.vty.verify("bsc-msc-lost-text MSC disconnected", ['']) self.vty.verify("bsc-welcome-text Hello MS", ['']) + self.vty.verify("bsc-grace-text In grace period", ['']) # Verify settings res = self.vty.command("write terminal") @@ -226,17 +228,46 @@ class TestVTYBSC(TestVTYGenericBSC): self.assertEquals(res.find('no bsc-msc-lost-text'), -1) self.assert_(res.find('bsc-welcome-text Hello MS') > 0) self.assertEquals(res.find('no bsc-welcome-text'), -1) + self.assert_(res.find('bsc-grace-text In grace period') > 0) + self.assertEquals(res.find('no bsc-grace-text'), -1) # Now disable it.. self.vty.verify("no bsc-msc-lost-text", ['']) self.vty.verify("no bsc-welcome-text", ['']) + self.vty.verify("no bsc-grace-text", ['']) # Verify settings res = self.vty.command("write terminal") self.assertEquals(res.find('bsc-msc-lost-text MSC disconnected'), -1) self.assert_(res.find('no bsc-msc-lost-text') > 0) - self.assert_(res.find('no bsc-welcome-text') > 0) self.assertEquals(res.find('bsc-welcome-text Hello MS'), -1) + self.assert_(res.find('no bsc-welcome-text') > 0) + self.assertEquals(res.find('bsc-grace-text In grace period'), -1) + self.assert_(res.find('no bsc-grace-text') > 0) + + def testUssdNotificationsBsc(self): + self.vty.enable() + self.vty.command("configure terminal") + self.vty.command("bsc") + + # Test invalid input + self.vty.verify("missing-msc-text", ['% Command incomplete.']) + + # Enable USSD notifications + self.vty.verify("missing-msc-text No MSC found", ['']) + + # Verify settings + res = self.vty.command("write terminal") + self.assert_(res.find('missing-msc-text No MSC found') > 0) + self.assertEquals(res.find('no missing-msc-text'), -1) + + # Now disable it.. + self.vty.verify("no missing-msc-text", ['']) + + # Verify settings + res = self.vty.command("write terminal") + self.assertEquals(res.find('missing-msc-text No MSC found'), -1) + self.assert_(res.find('no missing-msc-text') > 0) class TestVTYNAT(TestVTYGenericBSC): -- 1.7.9.5
This prevents the application from crashing when there is a half configured BTS (e.g. by using the command 'bts 1' when there isn't a BTS 1) and the 'write' command is used. --- openbsc/src/libbsc/bsc_vty.c | 10 ++++++---- openbsc/tests/vty_test_runner.py | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/openbsc/src/libbsc/bsc_vty.c b/openbsc/src/libbsc/bsc_vty.c index 5748945..55564b6 100644 --- a/openbsc/src/libbsc/bsc_vty.c +++ b/openbsc/src/libbsc/bsc_vty.c @@ -598,11 +598,13 @@ static void config_write_bts_single(struct vty *vty, struct gsm_bts *bts) if (bts->excl_from_rf_lock) vty_out(vty, " rf-lock-exclude%s", VTY_NEWLINE); - if (bts->model->config_write_bts) - bts->model->config_write_bts(vty, bts); + if (bts->model) { + if (bts->model->config_write_bts) + bts->model->config_write_bts(vty, bts); - llist_for_each_entry(trx, &bts->trx_list, list) - config_write_trx_single(vty, trx); + llist_for_each_entry(trx, &bts->trx_list, list) + config_write_trx_single(vty, trx); + } } static int config_write_bts(struct vty *v) diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index 460b70e..7f71288 100644 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -88,6 +88,17 @@ class TestVTYGenericBSC(TestVTYBase): self.assertTrue(self.vty.verify("trx 0",[''])) self.assertEquals(self.vty.node(), 'config-net-bts-trx') self.checkForEndAndExit() + self.vty.command("write terminal") + self.assertTrue(self.vty.verify("exit",[''])) + self.assertEquals(self.vty.node(), 'config-net-bts') + self.assertTrue(self.vty.verify("exit",[''])) + self.assertTrue(self.vty.verify("bts 1",[''])) + self.assertEquals(self.vty.node(), 'config-net-bts') + self.checkForEndAndExit() + self.assertTrue(self.vty.verify("trx 1",[''])) + self.assertEquals(self.vty.node(), 'config-net-bts-trx') + self.checkForEndAndExit() + self.vty.command("write terminal") self.assertTrue(self.vty.verify("exit",[''])) self.assertEquals(self.vty.node(), 'config-net-bts') self.assertTrue(self.vty.verify("exit",[''])) -- 1.7.9.5
Jacob Erlbeck wrote:
+++ b/openbsc/src/libbsc/bsc_vty.c @@ -598,11 +598,13 @@ static void config_write_bts_single(struct vty *vty, struct gsm_bts *bts) if (bts->excl_from_rf_lock) vty_out(vty, " rf-lock-exclude%s", VTY_NEWLINE);
- if (bts->model->config_write_bts) - bts->model->config_write_bts(vty, bts); + if (bts->model) { + if (bts->model->config_write_bts) + bts->model->config_write_bts(vty, bts);
- llist_for_each_entry(trx, &bts->trx_list, list) - config_write_trx_single(vty, trx); + llist_for_each_entry(trx, &bts->trx_list, list) + config_write_trx_single(vty, trx); + } }
My personal taste is to test all required conditions and exit early if they aren't met. That way, it's easy to get an overview of all conditions, before the "meat" of the function takes place. It also avoids one level of nesting for each condition. The patch would in this case become quite simple: + if (!bts->model) + return; (Maybe even if (!bts || !bts->model) ..) //Peter
On 09/12/2013 05:23 AM, Peter Stuge wrote:
My personal taste is to test all required conditions and exit early if they aren't met.
That way, it's easy to get an overview of all conditions, before the "meat" of the function takes place. It also avoids one level of nesting for each condition. The patch would in this case become quite simple:
+ if (!bts->model) + return;
(Maybe even if (!bts || !bts->model) ..)
I agree, that's much better. Thanks for looking into it. Jacob
This stores the last SET rf_locked control command along with a timestamp. The 'show network' vty command is extended to show this information. Ticket: OW#659 --- openbsc/include/openbsc/osmo_bsc_rf.h | 2 ++ openbsc/src/libbsc/bsc_rf_ctrl.c | 1 + openbsc/src/libbsc/bsc_vty.c | 4 ++++ openbsc/src/osmo-bsc/osmo_bsc_ctrl.c | 18 +++++++++++++++--- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/openbsc/include/openbsc/osmo_bsc_rf.h b/openbsc/include/openbsc/osmo_bsc_rf.h index a67e1bd..19ccd08 100644 --- a/openbsc/include/openbsc/osmo_bsc_rf.h +++ b/openbsc/include/openbsc/osmo_bsc_rf.h @@ -33,6 +33,8 @@ struct osmo_bsc_rf { const char *last_state_command; + char *last_rf_lock_ctrl_command; + /* delay the command */ char last_request; struct osmo_timer_list delay_cmd; diff --git a/openbsc/src/libbsc/bsc_rf_ctrl.c b/openbsc/src/libbsc/bsc_rf_ctrl.c index bd36e18..bce91e4 100644 --- a/openbsc/src/libbsc/bsc_rf_ctrl.c +++ b/openbsc/src/libbsc/bsc_rf_ctrl.c @@ -504,6 +504,7 @@ struct osmo_bsc_rf *osmo_bsc_rf_create(const char *path, struct gsm_network *net rf->gsm_network = net; rf->policy = S_RF_ON; rf->last_state_command = ""; + rf->last_rf_lock_ctrl_command = talloc_asprintf(rf, ""); /* check the rf state */ rf->rf_check.data = rf; diff --git a/openbsc/src/libbsc/bsc_vty.c b/openbsc/src/libbsc/bsc_vty.c index 55564b6..3fb8516 100644 --- a/openbsc/src/libbsc/bsc_vty.c +++ b/openbsc/src/libbsc/bsc_vty.c @@ -210,6 +210,10 @@ static void net_dump_vty(struct vty *vty, struct gsm_network *net) vty_out(vty, " Last RF Command: %s%s", net->bsc_data->rf_ctrl->last_state_command, VTY_NEWLINE); + if (net->bsc_data && net->bsc_data->rf_ctrl) + vty_out(vty, " Last RF Lock Command: %s%s", + net->bsc_data->rf_ctrl->last_rf_lock_ctrl_command, + VTY_NEWLINE); } DEFUN(show_net, show_net_cmd, "show network", diff --git a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c index 72abcf4..4e104db 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c @@ -35,6 +35,8 @@ #include <time.h> #include <unistd.h> +#define TIME_FORMAT_RFC2822 "%a, %d %b %Y %T %z" + void osmo_bsc_send_trap(struct ctrl_cmd *cmd, struct bsc_msc_connection *msc_con) { struct ctrl_cmd *trap; @@ -420,18 +422,28 @@ static int set_net_rf_lock(struct ctrl_cmd *cmd, void *data) { int locked = atoi(cmd->value); struct gsm_network *net = cmd->node; + time_t now = time(NULL); + char now_buf[64]; + struct osmo_bsc_rf *rf; + if (!net) { cmd->reply = "net not found."; return CTRL_CMD_ERROR; } - if (!net->bsc_data->rf_ctrl) { + rf = net->bsc_data->rf_ctrl; + + if (!rf) { cmd->reply = "RF Ctrl is not enabled in the BSC Configuration"; return CTRL_CMD_ERROR; } - osmo_bsc_rf_schedule_lock(net->bsc_data->rf_ctrl, - locked == 1 ? '0' : '1'); + talloc_free(rf->last_rf_lock_ctrl_command); + strftime(now_buf, sizeof(now_buf), TIME_FORMAT_RFC2822, gmtime(&now)); + rf->last_rf_lock_ctrl_command = + talloc_asprintf(rf, "rf_locked %u (%s)", locked, now_buf); + + osmo_bsc_rf_schedule_lock(rf, locked == 1 ? '0' : '1'); cmd->reply = talloc_asprintf(cmd, "%u", locked); if (!cmd->reply) { -- 1.7.9.5
On Wed, Sep 11, 2013 at 10:46:57AM +0200, Jacob Erlbeck wrote:
+ rf->last_rf_lock_ctrl_command = talloc_asprintf(rf, "");
GCC warns about this: bsc_rf_ctrl.c: In function ‘osmo_bsc_rf_create’: bsc_rf_ctrl.c:507:2: warning: zero-length gnu_printf format string [-Wformat-zero-length] the alternative is a strdup. I have modified it.
When verification failed and the reply string was not updated, the message "Someone forgot to fill in the reply." was shown instead of the default "Value failed verification." message. This patch modifies the implementation to set the default message if and only if verification fails and the reply hasn't been changed. --- openbsc/src/libctrl/control_cmd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openbsc/src/libctrl/control_cmd.c b/openbsc/src/libctrl/control_cmd.c index 3c4efc0..4d93c75 100644 --- a/openbsc/src/libctrl/control_cmd.c +++ b/openbsc/src/libctrl/control_cmd.c @@ -135,10 +135,12 @@ int ctrl_cmd_exec(vector vline, struct ctrl_cmd *command, vector node, void *dat goto out; } if (cmd_el->verify) { + const char *old_reply = command->reply; + if ((ret = cmd_el->verify(command, command->value, data))) { ret = CTRL_CMD_ERROR; /* If verify() set an appropriate error message, don't change it. */ - if (!command->reply) + if (command->reply == old_reply) command->reply = "Value failed verification."; goto out; } -- 1.7.9.5
On Wed, Sep 11, 2013 at 10:46:58AM +0200, Jacob Erlbeck wrote: Daniel,
When verification failed and the reply string was not updated, the message "Someone forgot to fill in the reply." was shown instead of the default "Value failed verification." message.
could you please comment on the patch?
This patch modifies the implementation to set the default message if and only if verification fails and the reply hasn't been changed. --- openbsc/src/libctrl/control_cmd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/openbsc/src/libctrl/control_cmd.c b/openbsc/src/libctrl/control_cmd.c index 3c4efc0..4d93c75 100644 --- a/openbsc/src/libctrl/control_cmd.c +++ b/openbsc/src/libctrl/control_cmd.c @@ -135,10 +135,12 @@ int ctrl_cmd_exec(vector vline, struct ctrl_cmd *command, vector node, void *dat goto out; } if (cmd_el->verify) { + const char *old_reply = command->reply; + if ((ret = cmd_el->verify(command, command->value, data))) { ret = CTRL_CMD_ERROR; /* If verify() set an appropriate error message, don't change it. */ - if (!command->reply) + if (command->reply == old_reply) command->reply = "Value failed verification."; goto out; } -- 1.7.9.5
On 09/11/2013 08:22 PM, Holger Hans Peter Freyther wrote:
On Wed, Sep 11, 2013 at 10:46:58AM +0200, Jacob Erlbeck wrote:
When verification failed and the reply string was not updated, the message "Someone forgot to fill in the reply." was shown instead of the default "Value failed verification." message. could you please comment on the patch?
This patch modifies the implementation to set the default message if and only if verification fails and the reply hasn't been changed. Having thought a little bit more about that, I'd rather modify ctrl_cmd_handle() to set cmd->reply at the end when it's still NULL and leave ctrl_cmd_exec() like it was:
--- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -147,7 +147,7 @@ int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) vector vline, cmdvec, cmds_vec; ret = CTRL_CMD_ERROR; - cmd->reply = "Someone forgot to fill in the reply."; + cmd->reply = NULL; node = CTRL_NODE_ROOT; cmd->node = net; @@ -238,6 +238,14 @@ int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) cmd_free_strvec(vline); err: + if (!cmd->reply) { + LOGP(DCTRL, LOGL_ERROR, "cmd->replay has not been set.\n", ret); + if (ret == CTRL_CMD_ERROR) + cmd->reply = "An error has occured."; + else + cmd->reply = "Command has been handled."; + } + if (ret == CTRL_CMD_ERROR) cmd->type = CTRL_TYPE_ERROR; return ret;
--- openbsc/src/libctrl/control_cmd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/openbsc/src/libctrl/control_cmd.c b/openbsc/src/libctrl/control_cmd.c index 3c4efc0..4d93c75 100644 --- a/openbsc/src/libctrl/control_cmd.c +++ b/openbsc/src/libctrl/control_cmd.c @@ -135,10 +135,12 @@ int ctrl_cmd_exec(vector vline, struct ctrl_cmd *command, vector node, void *dat goto out; } if (cmd_el->verify) { + const char *old_reply = command->reply; + if ((ret = cmd_el->verify(command, command->value, data))) { ret = CTRL_CMD_ERROR; /* If verify() set an appropriate error message, don't change it. */ - if (!command->reply) + if (command->reply == old_reply) command->reply = "Value failed verification."; goto out; } -- 1.7.9.5
Hi, On Thu, 2013-09-12 at 10:40, Jacob Erlbeck wrote:
On 09/11/2013 08:22 PM, Holger Hans Peter Freyther wrote:
On Wed, Sep 11, 2013 at 10:46:58AM +0200, Jacob Erlbeck wrote:
When verification failed and the reply string was not updated, the message "Someone forgot to fill in the reply." was shown instead of the default "Value failed verification." message. could you please comment on the patch?
This patch modifies the implementation to set the default message if and only if verification fails and the reply hasn't been changed. Having thought a little bit more about that, I'd rather modify ctrl_cmd_handle() to set cmd->reply at the end when it's still NULL and leave ctrl_cmd_exec() like it was:
this looks better, yeah.
--- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -147,7 +147,7 @@ int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) vector vline, cmdvec, cmds_vec;
ret = CTRL_CMD_ERROR; - cmd->reply = "Someone forgot to fill in the reply."; + cmd->reply = NULL; node = CTRL_NODE_ROOT; cmd->node = net;
@@ -238,6 +238,14 @@ int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) cmd_free_strvec(vline);
err: + if (!cmd->reply) { + LOGP(DCTRL, LOGL_ERROR, "cmd->replay has not been set.\n", ret); + if (ret == CTRL_CMD_ERROR) + cmd->reply = "An error has occured."; + else + cmd->reply = "Command has been handled."; + } + if (ret == CTRL_CMD_ERROR) cmd->type = CTRL_TYPE_ERROR; return ret;
Regards, - Daniel Willmann <dwillmann@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Schivelbeiner Str. 5 * 10439 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
On Fri, Sep 13, 2013 at 09:44:05AM +0200, Daniel Willmann wrote:
this looks better, yeah.
Dear Jacob, should I use the commit message from the first patch with the diff from your re-worked change? holger
This script is similar to vty_test_runner.py but tests the control interface instead. It currently tests some error cases, BTS status queries, and setting/clearing rf_locked. --- openbsc/tests/Makefile.am | 3 +- openbsc/tests/ctrl_test_runner.py | 240 +++++++++++++++++++++++++++++++++++++ 2 files changed, 242 insertions(+), 1 deletion(-) create mode 100644 openbsc/tests/ctrl_test_runner.py diff --git a/openbsc/tests/Makefile.am b/openbsc/tests/Makefile.am index 0597c14..f2dc057 100644 --- a/openbsc/tests/Makefile.am +++ b/openbsc/tests/Makefile.am @@ -27,7 +27,7 @@ $(srcdir)/package.m4: $(top_srcdir)/configure.ac echo ' [$(PACKAGE_URL)])'; \ } >'$(srcdir)/package.m4' -EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) vty_test_runner.py +EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) vty_test_runner.py ctrl_test_runner.py TESTSUITE = $(srcdir)/testsuite DISTCLEANFILES = atconfig @@ -36,6 +36,7 @@ python-tests: $(BUILT_SOURCES) osmotestvty.py -p $(abs_top_srcdir) -w $(abs_top_builddir) -v osmotestconfig.py -p $(abs_top_srcdir) -w $(abs_top_builddir) -v $(PYTHON) $(srcdir)/vty_test_runner.py -w $(abs_top_builddir) -v + $(PYTHON) $(srcdir)/ctrl_test_runner.py -w $(abs_top_builddir) -v rm -f $(top_builddir)/hlr.sqlite3 else python-tests: $(BUILT_SOURCES) diff --git a/openbsc/tests/ctrl_test_runner.py b/openbsc/tests/ctrl_test_runner.py new file mode 100644 index 0000000..8f5a0ec --- /dev/null +++ b/openbsc/tests/ctrl_test_runner.py @@ -0,0 +1,240 @@ +#!/usr/bin/env python + +# (C) 2013 by Jacob Erlbeck <jerlbeck@sysmocom.de> +# based on vty_test_runner.py: +# (C) 2013 by Katerina Barone-Adesi <kat.obsc@gmail.com> +# (C) 2013 by Holger Hans Peter Freyther +# based on bsc_control.py. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +import os +import time +import unittest +import socket +import sys +import struct + +import osmopy.obscvty as obscvty +import osmopy.osmoutil as osmoutil + +confpath = '.' +verbose = False + +class TestCtrlBase(unittest.TestCase): + + def ctrl_command(self): + raise Exception("Needs to be implemented by a subclass") + + def ctrl_app(self): + raise Exception("Needs to be implemented by a subclass") + + def setUp(self): + osmo_ctrl_cmd = self.ctrl_command()[:] + config_index = osmo_ctrl_cmd.index('-c') + if config_index: + cfi = config_index + 1 + osmo_ctrl_cmd[cfi] = os.path.join(confpath, osmo_ctrl_cmd[cfi]) + + try: + print "Launch: %s from %s" % (' '.join(osmo_ctrl_cmd), os.getcwd()) + self.proc = osmoutil.popen_devnull(osmo_ctrl_cmd) + except OSError: + print >> sys.stderr, "Current directory: %s" % os.getcwd() + print >> sys.stderr, "Consider setting -b" + time.sleep(2) + + appstring = self.ctrl_app()[2] + appport = self.ctrl_app()[0] + self.connect("127.0.0.1", appport) + self.next_id = 1000 + + def tearDown(self): + self.disconnect() + osmoutil.end_proc(self.proc) + + def prefix_ipa_ctrl_header(self, data): + return struct.pack(">HBB", len(data)+1, 0xee, 0) + data + + def remove_ipa_ctrl_header(self, data): + if (len(data) < 4): + raise BaseException("Answer too short!") + (plen, ipa_proto, osmo_proto) = struct.unpack(">HBB", data[:4]) + if (plen + 3 > len(data)): + print "Warning: Wrong payload length (expected %i, got %i)" % (plen, len(data) - 3) + if (ipa_proto != 0xee or osmo_proto != 0): + raise BaseException("Wrong protocol in answer!") + + return data[4:plen+3], data[plen+3:] + + def disconnect(self): + if not (self.sock is None): + self.sock.close() + + def connect(self, host, port): + if verbose: + print "Connecting to host %s:%i" % (host, port) + + sck = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sck.setblocking(1) + sck.connect((host, port)) + self.sock = sck + return sck + + def send(self, data): + if verbose: + print "Sending \"%s\"" %(data) + data = self.prefix_ipa_ctrl_header(data) + return self.sock.send(data) == len(data) + + def send_set(self, var, value, id): + setmsg = "SET %s %s %s" %(id, var, value) + return self.send(setmsg) + + def send_get(self, var, id): + getmsg = "GET %s %s" %(id, var) + return self.send(getmsg) + + def do_set(self, var, value): + id = self.next_id + self.next_id += 1 + self.send_set(var, value, id) + return self.recv_msgs()[id] + + def do_get(self, var): + id = self.next_id + self.next_id += 1 + self.send_get(var, id) + return self.recv_msgs()[id] + + def recv_msgs(self): + responses = {} + data = self.sock.recv(4096) + while (len(data)>0): + (answer, data) = self.remove_ipa_ctrl_header(data) + if verbose: + print "Got message:", answer + (mtype, id, msg) = answer.split(None, 2) + id = int(id) + rsp = {'mtype': mtype, 'id': id} + if mtype == "ERROR": + rsp['error'] = msg + else: + [rsp['var'], rsp['value']] = msg.split(None, 2) + + responses[id] = rsp + + if verbose: + print "Decoded replies: ", responses + + return responses + + +class TestCtrlBSC(TestCtrlBase): + + def tearDown(self): + TestCtrlBase.tearDown(self) + os.unlink("tmp_dummy_sock") + + def ctrl_command(self): + return ["./src/osmo-bsc/osmo-bsc", "-r", "tmp_dummy_sock", "-c", + "doc/examples/osmo-bsc/osmo-bsc.cfg"] + + def ctrl_app(self): + return (4249, "./src/osmo-bsc/osmo-bsc", "OsmoBSC", "bsc") + + def testCtrlErrs(self): + r = self.do_get('invalid') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Command not found') + + r = self.do_get('bts') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Error while parsing the index.') + + r = self.do_get('bts.999') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Error while resolving object') + + def testRfLock(self): + r = self.do_get('bts.0.rf_state') + self.assertEquals(r['mtype'], 'GET_REPLY') + self.assertEquals(r['var'], 'bts.0.rf_state') + self.assertEquals(r['value'], 'inoperational,unlocked,on') + + r = self.do_set('rf_locked', '1') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'rf_locked') + self.assertEquals(r['value'], '1') + + time.sleep(1.5) + + r = self.do_get('bts.0.rf_state') + self.assertEquals(r['mtype'], 'GET_REPLY') + self.assertEquals(r['var'], 'bts.0.rf_state') + self.assertEquals(r['value'], 'inoperational,locked,off') + + r = self.do_set('rf_locked', '0') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'rf_locked') + self.assertEquals(r['value'], '0') + + time.sleep(1.5) + + r = self.do_get('bts.0.rf_state') + self.assertEquals(r['mtype'], 'GET_REPLY') + self.assertEquals(r['var'], 'bts.0.rf_state') + self.assertEquals(r['value'], 'inoperational,unlocked,on') + +def add_bsc_test(suite, workdir): + if not os.path.isfile(os.path.join(workdir, "src/osmo-bsc/osmo-bsc")): + print("Skipping the BSC test") + return + test = unittest.TestLoader().loadTestsFromTestCase(TestCtrlBSC) + suite.addTest(test) + +if __name__ == '__main__': + import argparse + import sys + + workdir = '.' + + parser = argparse.ArgumentParser() + parser.add_argument("-v", "--verbose", dest="verbose", + action="store_true", help="verbose mode") + parser.add_argument("-p", "--pythonconfpath", dest="p", + help="searchpath for config") + parser.add_argument("-w", "--workdir", dest="w", + help="Working directory") + args = parser.parse_args() + + verbose_level = 1 + if args.verbose: + verbose_level = 2 + verbose = True + + if args.w: + workdir = args.w + + if args.p: + confpath = args.p + + print "confpath %s, workdir %s" % (confpath, workdir) + os.chdir(workdir) + print "Running tests for specific control commands" + suite = unittest.TestSuite() + add_bsc_test(suite, workdir) + res = unittest.TextTestRunner(verbosity=verbose_level).run(suite) + sys.exit(len(res.errors) + len(res.failures)) + -- 1.7.9.5
When verification failed and the reply string was not updated, the message "Someone forgot to fill in the reply." was shown instead of the default "Value failed verification." message. This patch changes the default reply handling in ctrl_cmd_handle() by setting the reply to NULL initially and then checking it at the end. If it hasn't been set, a generic message is assigned and an error is logged. --- openbsc/src/libctrl/control_if.c | 10 +++++++++- openbsc/tests/ctrl_test_runner.py | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/openbsc/src/libctrl/control_if.c b/openbsc/src/libctrl/control_if.c index b31f34f..2076f0c 100644 --- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -147,7 +147,7 @@ int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) vector vline, cmdvec, cmds_vec; ret = CTRL_CMD_ERROR; - cmd->reply = "Someone forgot to fill in the reply."; + cmd->reply = NULL; node = CTRL_NODE_ROOT; cmd->node = net; @@ -238,6 +238,14 @@ int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) cmd_free_strvec(vline); err: + if (!cmd->reply) { + LOGP(DCTRL, LOGL_ERROR, "cmd->replay has not been set.\n", ret); + if (ret == CTRL_CMD_ERROR) + cmd->reply = "An error has occured."; + else + cmd->reply = "Command has been handled."; + } + if (ret == CTRL_CMD_ERROR) cmd->type = CTRL_TYPE_ERROR; return ret; diff --git a/openbsc/tests/ctrl_test_runner.py b/openbsc/tests/ctrl_test_runner.py index 8f5a0ec..b012981 100644 --- a/openbsc/tests/ctrl_test_runner.py +++ b/openbsc/tests/ctrl_test_runner.py @@ -159,6 +159,10 @@ class TestCtrlBSC(TestCtrlBase): self.assertEquals(r['mtype'], 'ERROR') self.assertEquals(r['error'], 'Command not found') + r = self.do_set('rf_locked', '999') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Value failed verification.') + r = self.do_get('bts') self.assertEquals(r['mtype'], 'ERROR') self.assertEquals(r['error'], 'Error while parsing the index.') -- 1.7.9.5
On Wed, Sep 11, 2013 at 10:46:55AM +0200, Jacob Erlbeck wrote: Good Evening, these patches look fine! The only things I have is coding style. I understand that it takes some time to get used to it.
+ if (! text)
no space
+ if (! msg || msgb_l3len(msg) < sizeof(*gh)) { + return; + }
nu curly braces for single instructions, no space
+ bsc_send_ussd_notification(conn, msg, conn->bts->network->bsc_data->ussd_no_msc_txt);
Maybe we should start to save the bsc_data in the subscriber connection as well?
+ if (ret != BSC_CON_SUCCESS) { + /* allocation has failed */ + if (ret == BSC_CON_REJECT_NO_LINK) { + bsc_send_ussd_notification(conn, msg, msc->ussd_msc_lost_txt); + } else if (ret == BSC_CON_REJECT_RF_GRACE) { + bsc_send_ussd_notification(conn, msg, msc->ussd_grace_txt); + }
no curly braces.
+ if (!msc || !msc->msc_con->is_authenticated) { + // VSAT link down
left over of debug changes/information leak. I will try to fix these warnings right now. You will not need to send another round of patches.
participants (4)
-
Daniel Willmann -
Holger Hans Peter Freyther -
Jacob Erlbeck -
Peter Stuge