[PATCH] ctrl: Fix handling of missing replies

[Holger Hans Peter Freyther]

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