ctrl: Fix handling of missing replies
diff mbox

Message ID 1399649136-15337-1-git-send-email-jerlbeck@sysmocom.de
State Changes Requested
Headers show

Commit Message

Jacob Erlbeck May 9, 2014, 3:25 p.m. UTC
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(-)

Comments

Holger Freyther May 14, 2014, 5:25 a.m. UTC | #1
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

Patch
diff mbox

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;
 }