[PATCH] libosmocore[master]: ctrl: on parse errors, return a detailed message to sender

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/5437

ctrl: on parse errors, return a detailed message to sender

The recently added ctrl_cmd_parse2() returns non-NULL cmd with error messages
upon parsing errors. In handle_control_read(), use ctrl_cmd_parse2() and send
those back to the CTRL command sender as reply.

Retain the previous "Command parser error" reply only in case ctrl_cmd_parse2()
should return NULL, which shouldn't actually happen at all.

Change-Id: Ie35a02555b76913bb12734a76fc40fde7ffb244d
---
M src/ctrl/control_if.c
M tests/ctrl/ctrl_test.c
2 files changed, 25 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/37/5437/1

diff --git a/src/ctrl/control_if.c b/src/ctrl/control_if.c
index 5c73b63..17a012a 100644
--- a/src/ctrl/control_if.c
+++ b/src/ctrl/control_if.c
@@ -381,14 +381,10 @@
 
        msg->l2h = iph_ext->data;
 
-       cmd = ctrl_cmd_parse(ccon, msg);
+       cmd = ctrl_cmd_parse2(ccon, msg);
 
-       if (cmd) {
-               cmd->ccon = ccon;
-               if (ctrl_cmd_handle(ctrl, cmd, ctrl->data) != CTRL_CMD_HANDLED) 
{
-                       ctrl_cmd_send(&ccon->write_queue, cmd);
-               }
-       } else {
+       if (!cmd) {
+               /* should never happen */
                cmd = talloc_zero(ccon, struct ctrl_cmd);
                if (!cmd)
                        return -ENOMEM;
@@ -396,10 +392,23 @@
                cmd->type = CTRL_TYPE_ERROR;
                cmd->id = "err";
                cmd->reply = "Command parser error.";
-               ctrl_cmd_send(&ccon->write_queue, cmd);
        }
 
-       talloc_free(cmd);
+       if (cmd->type != CTRL_TYPE_ERROR) {
+               cmd->ccon = ccon;
+               if (ctrl_cmd_handle(ctrl, cmd, ctrl->data) == CTRL_CMD_HANDLED) 
{
+                       /* On CTRL_CMD_HANDLED, no reply needs to be sent back. 
*/
+                       talloc_free(cmd);
+                       cmd = NULL;
+               }
+       }
+
+       if (cmd) {
+               /* There is a reply or error that should be reported back to 
the sender. */
+               ctrl_cmd_send(&ccon->write_queue, cmd);
+               talloc_free(cmd);
+       }
+
        return 0;
 }
 
@@ -894,13 +903,16 @@
        osmo_strlcpy((char *)msg->data, cmdstr, msgb_tailroom(msg));
        msgb_put(msg, strlen(cmdstr));
 
-       cmd = ctrl_cmd_parse(ch, msg);
+       cmd = ctrl_cmd_parse2(ch, msg);
        msgb_free(msg);
        if (!cmd)
                return NULL;
-       if (ctrl_cmd_handle(ch, cmd, NULL) < 0) {
+       if (cmd->type == CTRL_TYPE_ERROR)
+               return cmd;
+       if (ctrl_cmd_handle(ch, cmd, NULL) == CTRL_CMD_HANDLED) {
+               /* No reply should be sent back. */
                talloc_free(cmd);
-               return NULL;
+               cmd = NULL;
        }
        return cmd;
 }
diff --git a/tests/ctrl/ctrl_test.c b/tests/ctrl/ctrl_test.c
index 86ede8b..7181db3 100644
--- a/tests/ctrl/ctrl_test.c
+++ b/tests/ctrl/ctrl_test.c
@@ -76,7 +76,7 @@
        printf("test: '%s'\n", osmo_escape_str(t->cmd_str, -1));
        printf("parsing:\n");
 
-       cmd = ctrl_cmd_parse(ctx, msg);
+       cmd = ctrl_cmd_parse2(ctx, msg);
        OSMO_ASSERT(cmd);
 
        OSMO_ASSERT(t->expect_parsed.type == cmd->type);

-- 
To view, visit https://gerrit.osmocom.org/5437
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie35a02555b76913bb12734a76fc40fde7ffb244d
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de>