From 462323f123ede2c8da59cb4acb772dc4f98dee79 Mon Sep 17 00:00:00 2001 From: Antonio Borneo Date: Tue, 12 May 2020 01:59:06 +0200 Subject: [PATCH] helper/command: use one single handler for all the commands Today openocd registers the commands to jim with three methods: 1) "native" commands (.jim_handler) at root level are registered directly as jim commands; 2) "simple" commands (.handler) at root level are registered through the handler script_command(); 3) all other commands not at root level are registered through the handler command_unknown(). Apart from using different handler, other inconsistencies are present: a) command in 1) are not checked for their "mode", so are run with no check about current mode (COMMAND_CONFIG or COMMAND_EXEC); b) target_call_timer_callbacks_now() is called only for "simple" commands and not for "native" commands; c) target override is performed only for "simple" commands and not for "native" commands. Drop script_command() and extend command_unknown() to uniformly handle all the cases above, fixing all the inconsistencies already mentioned. The handler's name command_unknown() is probably not anymore appropriate, but will be renamed in a separate change. Note: today all the commands in a) have mode CONFIG_ANY, apart for "mem2array" and "array2mem" that have mode COMMAND_EXEC. But the latter commands are registered during target init, so do not exist during COMMAND_CONFIG and no issue is present. Change-Id: I67bd6e47eb2c575107251b9192c676c27d4aabae Signed-off-by: Antonio Borneo Reviewed-on: http://openocd.zylin.com/5665 Tested-by: jenkins Reviewed-by: Oleksij Rempel --- src/helper/command.c | 120 ++++++++++++++++++------------------------- 1 file changed, 49 insertions(+), 71 deletions(-) diff --git a/src/helper/command.c b/src/helper/command.c index b44e4667f..ecca75db6 100644 --- a/src/helper/command.c +++ b/src/helper/command.c @@ -44,9 +44,6 @@ /* nice short description of source file */ #define __THIS__FILE__ "command.c" -static int run_command(struct command_context *context, - struct command *c, const char *words[], unsigned num_words); - struct log_capture_state { Jim_Interp *interp; Jim_Obj *output; @@ -226,34 +223,6 @@ struct command_context *current_command_context(Jim_Interp *interp) return cmd_ctx; } -static int script_command_run(Jim_Interp *interp, - int argc, Jim_Obj * const *argv, struct command *c) -{ - target_call_timer_callbacks_now(); - LOG_USER_N("%s", ""); /* Keep GDB connection alive*/ - - unsigned nwords; - char **words = script_command_args_alloc(argc, argv, &nwords); - if (NULL == words) - return JIM_ERR; - - struct command_context *cmd_ctx = current_command_context(interp); - int retval = run_command(cmd_ctx, c, (const char **)words, nwords); - - script_command_args_free(words, nwords); - return command_retval_set(interp, retval); -} - -static int script_command(Jim_Interp *interp, int argc, Jim_Obj *const *argv) -{ - /* the private data is stashed in the interp structure */ - - struct command *c = jim_to_command(interp); - assert(c); - script_debug(interp, argc, argv); - return script_command_run(interp, argc, argv, c); -} - static struct command *command_root(struct command *c) { while (NULL != c->parent) @@ -387,10 +356,7 @@ static int register_command_handler(struct command_context *cmd_ctx, LOG_DEBUG("registering '%s'...", c->name); #endif - Jim_CmdProc *func = c->handler ? &script_command : &command_unknown; - int retval = Jim_CreateCommand(interp, c->name, func, c, NULL); - - return retval; + return Jim_CreateCommand(interp, c->name, command_unknown, c, NULL); } static struct command *register_command(struct command_context *context, @@ -415,16 +381,12 @@ static struct command *register_command(struct command_context *context, if (NULL == c) return NULL; - int retval = JIM_OK; - if (NULL != cr->jim_handler && NULL == parent) { - retval = Jim_CreateCommand(context->interp, cr->name, - cr->jim_handler, c, NULL); - } else if (NULL != cr->handler || NULL != parent) - retval = register_command_handler(context, command_root(c)); - - if (retval != JIM_OK) { - unregister_command(context, parent, name); - c = NULL; + if (cr->jim_handler || cr->handler) { + int retval = register_command_handler(context, command_root(c)); + if (retval != JIM_OK) { + unregister_command(context, parent, name); + return NULL; + } } return c; } @@ -614,11 +576,8 @@ static bool command_can_run(struct command_context *cmd_ctx, struct command *c) } static int run_command(struct command_context *context, - struct command *c, const char *words[], unsigned num_words) + struct command *c, const char **words, unsigned num_words) { - if (!command_can_run(context, c)) - return ERROR_FAIL; - struct command_invocation cmd = { .ctx = context, .current = c, @@ -626,26 +585,11 @@ static int run_command(struct command_context *context, .argc = num_words - 1, .argv = words + 1, }; - /* Black magic of overridden current target: - * If the command we are going to handle has a target prefix, - * override the current target temporarily for the time - * of processing the command. - * current_target_override is used also for event handlers - * therefore we prevent touching it if command has no prefix. - * Previous override is saved and restored back to ensure - * correct work when run_command() is re-entered. */ - struct target *saved_target_override = context->current_target_override; - 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) - context->current_target_override = saved_target_override; - if (retval == ERROR_COMMAND_SYNTAX_ERROR) { /* Print help for command */ char *full_name = command_name(c, ' '); @@ -1053,6 +997,23 @@ static int run_usage(Jim_Interp *interp, int argc_valid, int argc, Jim_Obj * con return retval; } +static int exec_command(Jim_Interp *interp, struct command_context *cmd_ctx, + struct command *c, int argc, Jim_Obj *const *argv) +{ + if (c->jim_handler) + return c->jim_handler(interp, argc, argv); + + /* use c->handler */ + unsigned int nwords; + char **words = script_command_args_alloc(argc, argv, &nwords); + if (!words) + return JIM_ERR; + + int retval = run_command(cmd_ctx, c, (const char **)words, nwords); + script_command_args_free(words, nwords); + return command_retval_set(interp, retval); +} + static int command_unknown(Jim_Interp *interp, int argc, Jim_Obj *const *argv) { script_debug(interp, argc, argv); @@ -1079,15 +1040,32 @@ static int command_unknown(Jim_Interp *interp, int argc, Jim_Obj *const *argv) run_usage(interp, count, argc, start); return JIM_ERR; } - /* pass the command through to the intended handler */ - if (c->jim_handler) { - if (!command_can_run(cmd_ctx, c)) - return JIM_ERR; - return (*c->jim_handler)(interp, count, start); - } + if (!command_can_run(cmd_ctx, c)) + return JIM_ERR; - return script_command_run(interp, count, start, c); + target_call_timer_callbacks_now(); + + /* + * Black magic of overridden current target: + * If the command we are going to handle has a target prefix, + * override the current target temporarily for the time + * of processing the command. + * current_target_override is used also for event handlers + * therefore we prevent touching it if command has no prefix. + * Previous override is saved and restored back to ensure + * correct work when command_unknown() is re-entered. + */ + struct target *saved_target_override = cmd_ctx->current_target_override; + if (c->jim_handler_data) + cmd_ctx->current_target_override = c->jim_handler_data; + + int retval = exec_command(interp, cmd_ctx, c, count, start); + + if (c->jim_handler_data) + cmd_ctx->current_target_override = saved_target_override; + + return retval; } static int jim_command_mode(Jim_Interp *interp, int argc, Jim_Obj *const *argv)