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

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Mon Dec 18 03:39:22 UTC 2017


Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/5437

to look at the new patch set (#4).

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

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 b1d4f23..39ec61a 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: newpatchset
Gerrit-Change-Id: Ie35a02555b76913bb12734a76fc40fde7ffb244d
Gerrit-PatchSet: 4
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder



More information about the gerrit-log mailing list