Re: [PATCH 4/4] ctrl: Fix generic reply when verification fails

[Jacob Erlbeck]

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