[PATCH] ctrl: Fix handling of missing replies

[Jacob Erlbeck]

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


-- 
- Jacob Erlbeck <jerlbeck at 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