New AMI Command Output Format

This change modifies how the the output from a CLI command is sent
to a client over AMI.

Output from the CLI command is now sent as a series of zero-or-more
Output: headers.

Additionally, commands that fail to execute (eg: no such command,
invalid syntax etc.) now cause an Error response instead of Success.

If the command executed successfully, but the manager unable to
provide the output the reason will be included in the Message:
header. Otherwise it will contain 'Command output follows'.

Depends on a new version of starpy (> 1.0.2) that supports the new
output format.

See pull-request https://github.com/asterisk/starpy/pull/34

ASTERISK-24730

Change-Id: I6718d95490f0a6b3f171c1a5cdad9207f9a44888

4 files changed, 46 insertions, 34 deletions

diff --git a/UPGRADE.txt b/UPGRADE.txt
index 8a349f0df..0696adb29 100644
--- a/UPGRADE.txt
+++ b/UPGRADE.txt
@@ -46,5 +46,13 @@ Logging:
  - The first callid created is now 1 instead of 0.  The value 0
    is now reserved to represent a lack of callid.
 
+AMI:
+ - The Command action now sends the output from the CLI command as a series
+   of Output headers for each line instead of as a block of text with the
+   --END COMMAND-- delimiter to match the output from other actions.
+
+   Commands that fail to execute (no such command, invalid syntax etc.) now
+   return an Error response instead of Success.
+
 ===========================================================
 ===========================================================
diff --git a/include/asterisk/manager.h b/include/asterisk/manager.h
index 43031d1d6..b5ede545e 100644
--- a/include/asterisk/manager.h
+++ b/include/asterisk/manager.h
@@ -54,7 +54,7 @@
 - \ref manager.c Main manager code file
  */
 
-#define AMI_VERSION                     "2.7.0"
+#define AMI_VERSION                     "2.8.0"
 #define DEFAULT_MANAGER_PORT 5038	/* Default port for Asterisk management via TCP */
 #define DEFAULT_MANAGER_TLS_PORT 5039	/* Default port for Asterisk management via TCP */
 
diff --git a/main/cli.c b/main/cli.c
index 9d3cdf3f4..a230c20ac 100644
--- a/main/cli.c
+++ b/main/cli.c
@@ -2681,12 +2681,12 @@ int ast_cli_command_full(int uid, int gid, int fd, const char *s)
 	int x;
 	char *duplicate = parse_args(s, &x, args + 1, AST_MAX_ARGS, NULL);
 	char tmp[AST_MAX_ARGS + 1];
-	char *retval = NULL;
+	char *retval = CLI_FAILURE;
 	struct ast_cli_args a = {
 		.fd = fd, .argc = x, .argv = args+1 };
 
 	if (duplicate == NULL)
-		return -1;
+		return RESULT_FAILURE;
 
 	if (x < 1)	/* We need at least one entry, otherwise ignore */
 		goto done;
@@ -2705,8 +2705,7 @@ int ast_cli_command_full(int uid, int gid, int fd, const char *s)
 	/* Check if the user has rights to run this command. */
 	if (!cli_has_permissions(uid, gid, tmp)) {
 		ast_cli(fd, "You don't have permissions to run '%s' command\n", tmp);
-		ast_free(duplicate);
-		return 0;
+		goto done;
 	}
 
 	/*
@@ -2719,14 +2718,13 @@ int ast_cli_command_full(int uid, int gid, int fd, const char *s)
 
 	if (retval == CLI_SHOWUSAGE) {
 		ast_cli(fd, "%s", S_OR(e->usage, "Invalid usage, but no usage information available.\n"));
-	} else {
-		if (retval == CLI_FAILURE)
-			ast_cli(fd, "Command '%s' failed.\n", s);
+	} else if (retval == CLI_FAILURE) {
+		ast_cli(fd, "Command '%s' failed.\n", s);
 	}
 	ast_atomic_fetchadd_int(&e->inuse, -1);
 done:
 	ast_free(duplicate);
-	return 0;
+	return retval == CLI_SUCCESS ? RESULT_SUCCESS : RESULT_FAILURE;
 }
 
 int ast_cli_command_multiple_full(int uid, int gid, int fd, size_t size, const char *s)
diff --git a/main/manager.c b/main/manager.c
index f2a516f24..2ff9df930 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -4839,11 +4839,10 @@ static int check_blacklist(const char *cmd)
 static int action_command(struct mansession *s, const struct message *m)
 {
 	const char *cmd = astman_get_header(m, "Command");
-	const char *id = astman_get_header(m, "ActionID");
-	char *buf = NULL, *final_buf = NULL;
+	char *buf = NULL, *final_buf = NULL, *delim, *output;
 	char template[] = "/tmp/ast-ami-XXXXXX";	/* template for temporary file */
-	int fd;
-	off_t l;
+	int fd, ret;
+	off_t len;
 
 	if (ast_strlen_zero(cmd)) {
 		astman_send_error(s, m, "No command provided");
@@ -4856,52 +4855,59 @@ static int action_command(struct mansession *s, const struct message *m)
 	}
 
 	if ((fd = mkstemp(template)) < 0) {
-		ast_log(AST_LOG_WARNING, "Failed to create temporary file for command: %s\n", strerror(errno));
-		astman_send_error(s, m, "Command response construction error");
+		astman_send_error_va(s, m, "Failed to create temporary file: %s", strerror(errno));
 		return 0;
 	}
 
-	astman_append(s, "Response: Follows\r\nPrivilege: Command\r\n");
-	if (!ast_strlen_zero(id)) {
-		astman_append(s, "ActionID: %s\r\n", id);
-	}
-	/* FIXME: Wedge a ActionID response in here, waiting for later changes */
-	ast_cli_command(fd, cmd);	/* XXX need to change this to use a FILE * */
+	ret = ast_cli_command(fd, cmd);
+	astman_send_response_full(s, m, ret == RESULT_SUCCESS ? "Success" : "Error", MSG_MOREDATA, NULL);
+
 	/* Determine number of characters available */
-	if ((l = lseek(fd, 0, SEEK_END)) < 0) {
-		ast_log(LOG_WARNING, "Failed to determine number of characters for command: %s\n", strerror(errno));
+	if ((len = lseek(fd, 0, SEEK_END)) < 0) {
+		astman_append(s, "Message: Failed to determine number of characters: %s\r\n", strerror(errno));
 		goto action_command_cleanup;
 	}
 
 	/* This has a potential to overflow the stack.  Hence, use the heap. */
-	buf = ast_malloc(l + 1);
-	final_buf = ast_malloc(l + 1);
+	buf = ast_malloc(len + 1);
+	final_buf = ast_malloc(len + 1);
 
 	if (!buf || !final_buf) {
-		ast_log(LOG_WARNING, "Failed to allocate memory for temporary buffer\n");
+		astman_append(s, "Message: Memory allocation failure\r\n");
 		goto action_command_cleanup;
 	}
 
 	if (lseek(fd, 0, SEEK_SET) < 0) {
-		ast_log(LOG_WARNING, "Failed to set position on temporary file for command: %s\n", strerror(errno));
+		astman_append(s, "Message: Failed to set position on temporary file: %s\r\n", strerror(errno));
 		goto action_command_cleanup;
 	}
 
-	if (read(fd, buf, l) < 0) {
-		ast_log(LOG_WARNING, "read() failed: %s\n", strerror(errno));
+	if (read(fd, buf, len) < 0) {
+		astman_append(s, "Message: Failed to read from temporary file: %s\r\n", strerror(errno));
 		goto action_command_cleanup;
 	}
 
-	buf[l] = '\0';
-	term_strip(final_buf, buf, l);
-	final_buf[l] = '\0';
-	astman_append(s, "%s", final_buf);
+	buf[len] = '\0';
+	term_strip(final_buf, buf, len);
+	final_buf[len] = '\0';
+
+	/* Trim trailing newline */
+	if (len && final_buf[len - 1] == '\n') {
+		final_buf[len - 1] = '\0';
+	}
+
+	astman_append(s, "Message: Command output follows\r\n");
+
+	delim = final_buf;
+	while ((output = strsep(&delim, "\n"))) {
+		astman_append(s, "Output: %s\r\n", output);
+	}
 
 action_command_cleanup:
+	astman_append(s, "\r\n");
 
 	close(fd);
 	unlink(template);
-	astman_append(s, "--END COMMAND--\r\n\r\n");
 
 	ast_free(buf);
 	ast_free(final_buf);