helper/command: Handle Tcl return values consistently 15/1815/7
authorPaul Fertser <fercerpav@gmail.com>
Thu, 4 Apr 2019 07:54:09 +0000 (09:54 +0200)
committerTomas Vanek <vanekt@fbl.cz>
Tue, 14 May 2019 18:38:22 +0000 (19:38 +0100)
Rationale:

1. There's logging output and there're return values;

2. If a function should return something, it should do it explicitly,
same for logging;

3. Interactive interfaces (telnet, Gdb and Tcl RPC) must always return
the result of the evaluation for the given expression. You can
suppress this output by adding ``; after 0'' to the end of your
expression.

4. Some commands "throw an exception" and if you want to be able to
collect both the return value (when it succeeds) and the log output
(when something goes wrong) you can use do like this:

set log_output [capture "catch {reset_config} return_value"]

So what I'm proposing is following:

1. Every jim_handler command should set the return value the standard
JimTcl way, without any tricks. If it needs to print some logging
output, it should use LOG_* functions.

2. The usual commands (COMMANDS) can easily construct their return
value by appending strings with command_print() and
command_print_sameline(). This required changing "struct
command_invocation" and passing a pointer to it to command_print*
functions.

The code is already functional, please test and comment.

TODO items:

1. Modify all jim_handler commands to properly return or log values
(some of them are commented out for now in this patch).

2. Properly document "capture" command and provide a convenience
function to automate log_output + return_value gathering.

3. Add appropriate Doxygen comments.

4. Add Tcl RPC interface description to the manual, all the example
clients in different languages (from the mailing list) to contrib/.

This change is the core part of http://openocd.zylin.com/1815 from
Paul Fertser. It has been extracted and rebased to simplify the
review and provided again as 1815.

Change-Id: I675c91aa9da1e4e7c6f0a8fe6112a00550b9e4db
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Reviewed-on: http://openocd.zylin.com/1815
Tested-by: jenkins
src/helper/command.c
src/helper/command.h

index be131bb..11c9e5f 100644 (file)
@@ -510,13 +510,13 @@ void command_print_sameline(struct command_invocation *cmd, const char *format,
        va_start(ap, format);
 
        string = alloc_vprintf(format, ap);
-       if (string != NULL) {
+       if (string != NULL && cmd) {
                /* we want this collected in the log + we also want to pick it up as a tcl return
                 * value.
                 *
                 * The latter bit isn't precisely neat, but will do for now.
                 */
-               LOG_USER_N("%s", string);
+               Jim_AppendString(cmd->ctx->interp, cmd->output, string, -1);
                /* We already printed it above
                 * command_output_text(context, string); */
                free(string);
@@ -533,7 +533,7 @@ void command_print(struct command_invocation *cmd, const char *format, ...)
        va_start(ap, format);
 
        string = alloc_vprintf(format, ap);
-       if (string != NULL) {
+       if (string != NULL && cmd) {
                strcat(string, "\n");   /* alloc_vprintf guaranteed the buffer to be at least one
                                         *char longer */
                /* we want this collected in the log + we also want to pick it up as a tcl return
@@ -541,7 +541,7 @@ void command_print(struct command_invocation *cmd, const char *format, ...)
                 *
                 * The latter bit isn't precisely neat, but will do for now.
                 */
-               LOG_USER_N("%s", string);
+               Jim_AppendString(cmd->ctx->interp, cmd->output, string, -1);
                /* We already printed it above
                 * command_output_text(context, string); */
                free(string);
@@ -628,6 +628,9 @@ static int run_command(struct command_context *context,
        if (c->jim_handler_data)
                context->current_target_override = c->jim_handler_data;
 
+       cmd.output = Jim_NewEmptyStringObj(context->interp);
+       Jim_IncrRefCount(cmd.output);
+
        int retval = c->handler(&cmd);
 
        if (c->jim_handler_data)
@@ -650,7 +653,11 @@ static int run_command(struct command_context *context,
                LOG_DEBUG("Command '%s' failed with error code %d",
                                        full_name ? full_name : c->name, retval);
                free(full_name);
+       } else {
+               /* Use the command output as the Tcl result */
+               Jim_SetResult(context->interp, cmd.output);
        }
+       Jim_DecrRefCount(context->interp, cmd.output);
 
        return retval;
 }
index 41cdc0b..733ba42 100644 (file)
@@ -79,6 +79,7 @@ struct command_invocation {
        const char *name;
        unsigned argc;
        const char **argv;
+       Jim_Obj *output;
 };
 
 /**