Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set").
This patch changes the logging level from ERROR to INFO. The logging is now only done, when the retry has not been set and the implementation returns CTRL_CMD_ERROR or the GET has been used. This means, every time a error is signalled or GET is used, the retry field shall be set.
Some missing reply texts in error cases are also added.
Ticket: OW#1177 Sponsored-by: On-Waves ehf --- openbsc/src/libbsc/bsc_ctrl_lookup.c | 11 ++++++++--- openbsc/src/osmo-bsc/osmo_bsc_ctrl.c | 4 +++- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/openbsc/src/libbsc/bsc_ctrl_lookup.c b/openbsc/src/libbsc/bsc_ctrl_lookup.c index 338fb11..dced8bd 100644 --- a/openbsc/src/libbsc/bsc_ctrl_lookup.c +++ b/openbsc/src/libbsc/bsc_ctrl_lookup.c @@ -150,10 +150,15 @@ int bsc_ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data)
err: if (!cmd->reply) { - LOGP(DCTRL, LOGL_ERROR, "cmd->reply has not been set.\n"); - if (ret == CTRL_CMD_ERROR) + if (ret == CTRL_CMD_ERROR) { cmd->reply = "An error has occured."; - else + LOGP(DCTRL, LOGL_NOTICE, + "cmd->reply has not been set (ERROR).\n"); + } else if (cmd->type == CTRL_TYPE_GET) { + LOGP(DCTRL, LOGL_NOTICE, + "cmd->reply has not been set (GET).\n"); + cmd->reply = ""; + } else cmd->reply = "Command has been handled."; }
diff --git a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c index e32218d..3cc704b 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c @@ -72,6 +72,7 @@ static int get_msc_connection_status(struct ctrl_cmd *cmd, void *data)
static int set_msc_connection_status(struct ctrl_cmd *cmd, void *data) { + cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR; }
@@ -128,6 +129,7 @@ static int get_bts_connection_status(struct ctrl_cmd *cmd, void *data)
static int set_bts_connection_status(struct ctrl_cmd *cmd, void *data) { + cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR; }
@@ -522,7 +524,7 @@ static int get_bts_rf_state(struct ctrl_cmd *cmd, void *data)
static int set_bts_rf_state(struct ctrl_cmd *cmd, void *data) { - cmd->reply = "set is unimplemented"; + cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR; }
On Fri, May 09, 2014 at 05:25:36PM +0200, Jacob Erlbeck wrote:
Good Morning,
Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set").
hmm. I would like to see some more reflection here. The "reply" not being set being an ERROR was done by you, now the NAT might have a legetimate use-case for not setting a reply. I don't see this being reflected in the commit message.
if (!cmd->reply) {
LOGP(DCTRL, LOGL_ERROR, "cmd->reply has not been set.\n");if (ret == CTRL_CMD_ERROR)
if (ret == CTRL_CMD_ERROR) { cmd->reply = "An error has occured.";
else
LOGP(DCTRL, LOGL_NOTICE,"cmd->reply has not been set (ERROR).\n");} else if (cmd->type == CTRL_TYPE_GET) {LOGP(DCTRL, LOGL_NOTICE,"cmd->reply has not been set (GET).\n");cmd->reply = "";} else cmd->reply = "Command has been handled.";
I am not convinced. The NAT will still generate these messages on the stdout but there is nothing to be fixed?! To make the actual log message more useful one should output the name of command. :)
static int set_msc_connection_status(struct ctrl_cmd *cmd, void *data) {
- cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR;
}
Hmm, "unimplemented" somehow indicates that we would know what to implement. E.g. if we take a look at verify_msc_connection_status I claim that "msc_connection_status" is just a read-only attribute.
@@ -128,6 +129,7 @@ static int get_bts_connection_status(struct ctrl_cmd *cmd, void *data)
static int set_bts_connection_status(struct ctrl_cmd *cmd, void *data) {
- cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR;
}
The same here. It is read-only.
@@ -522,7 +524,7 @@ static int get_bts_rf_state(struct ctrl_cmd *cmd, void *data)
static int set_bts_rf_state(struct ctrl_cmd *cmd, void *data) {
- cmd->reply = "set is unimplemented";
- cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR;
}
It should probably be read-only. I will introduce a CTRL_CMD_DEFINE for read-only commands but the actual issue is still there.
cheers holger
Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set"). It even complains when the function implementing the command returns CTRL_CMD_HANDLED, where a reply text is not needed.
This patch changes the logging level from ERROR to NOTICE. The logging is now only done, when the retry has not been set and the implementation returns either CTRL_CMD_ERROR or CTRL_CMD_REPLY. So in these cases the reply field must be set.
This fixes the generation of log messages when doing NAT ctrl command forwarding.
Ticket: OW#1177 Sponsored-by: On-Waves ehf --- openbsc/src/libbsc/bsc_ctrl_lookup.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/openbsc/src/libbsc/bsc_ctrl_lookup.c b/openbsc/src/libbsc/bsc_ctrl_lookup.c index 338fb11..3a3df9c 100644 --- a/openbsc/src/libbsc/bsc_ctrl_lookup.c +++ b/openbsc/src/libbsc/bsc_ctrl_lookup.c @@ -150,11 +150,19 @@ int bsc_ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data)
err: if (!cmd->reply) { - LOGP(DCTRL, LOGL_ERROR, "cmd->reply has not been set.\n"); - if (ret == CTRL_CMD_ERROR) + if (ret == CTRL_CMD_ERROR) { cmd->reply = "An error has occured."; - else + LOGP(DCTRL, LOGL_NOTICE, + "%s: cmd->reply has not been set (ERROR).\n", + cmd->variable); + } else if (ret == CTRL_CMD_REPLY) { + LOGP(DCTRL, LOGL_NOTICE, + "%s: cmd->reply has not been set (type = %d).\n", + cmd->variable, cmd->type); + cmd->reply = ""; + } else { cmd->reply = "Command has been handled."; + } }
if (ret == CTRL_CMD_ERROR)
On Thu, May 15, 2014 at 01:04:14PM +0200, Jacob Erlbeck wrote:
Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set"). It even complains when the function implementing the command returns CTRL_CMD_HANDLED, where a reply text is not needed.
This patch changes the logging level from ERROR to NOTICE. The logging is now only done, when the retry has not been set and the
^^^^^^ <- reply?
implementation returns either CTRL_CMD_ERROR or CTRL_CMD_REPLY. So in these cases the reply field must be set.
if (ret == CTRL_CMD_ERROR)
if (ret == CTRL_CMD_ERROR) { cmd->reply = "An error has occured.";
else
LOGP(DCTRL, LOGL_NOTICE,"%s: cmd->reply has not been set (ERROR).\n",cmd->variable);} else if (ret == CTRL_CMD_REPLY) {LOGP(DCTRL, LOGL_NOTICE,"%s: cmd->reply has not been set (type = %d).\n",cmd->variable, cmd->type);cmd->reply = "";} else { cmd->reply = "Command has been handled.";
Is using a switch/case better here? So for CTRL_CMD_HANDLED the "Command has been handled" will be set?
On 15.05.2014 21:24, Holger Hans Peter Freyther wrote:
On Thu, May 15, 2014 at 01:04:14PM +0200, Jacob Erlbeck wrote:
Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set"). It even complains when the function implementing the command returns CTRL_CMD_HANDLED, where a reply text is not needed.
This patch changes the logging level from ERROR to NOTICE. The logging is now only done, when the retry has not been set and the
^^^^^^ <- reply?
Yes, of course, thanks.
implementation returns either CTRL_CMD_ERROR or CTRL_CMD_REPLY. So in these cases the reply field must be set.
if (ret == CTRL_CMD_ERROR)
if (ret == CTRL_CMD_ERROR) { cmd->reply = "An error has occured.";
else
LOGP(DCTRL, LOGL_NOTICE,"%s: cmd->reply has not been set (ERROR).\n",cmd->variable);} else if (ret == CTRL_CMD_REPLY) {LOGP(DCTRL, LOGL_NOTICE,"%s: cmd->reply has not been set (type = %d).\n",cmd->variable, cmd->type);cmd->reply = "";} else { cmd->reply = "Command has been handled.";Is using a switch/case better here? So for CTRL_CMD_HANDLED the "Command has been handled" will be set?
Yes, I'd like to use a switch here, too. But CTRL_CMD_* are not enums and CTRL_CMD_ERROR is defined to -1, which I don't want to use as case constant. I'd rather turn CTRL_CMD_* into an enum and replace if's by switches in another patch.
Jacob
-
Holger Hans Peter Freyther -
Jacob Erlbeck