From 428938993742f4f961cdc948593d9553f721c321 Mon Sep 17 00:00:00 2001 From: Antonio Borneo Date: Tue, 12 May 2020 11:52:56 +0200 Subject: [PATCH] helper/command: override target only on target prefixed cmds In current code the current target is overridden whenever jim_handler_data is not NULL. This happens not only with target prefixed commands, but also with cti, dap and swo/tpiu prefixed commands. While this is not causing any run-time issue, by now, the behaviour is tricky and makes the code cryptic. Add a specific field to struct command for the target override so the content of jim_handler_data can be restricted to command specific data only (today only cti, dap and swo/tpiu). Extend the API register_commands() to specify the presence of either the command data or the override target. The new API makes obsolete calling command_set_handler_data() to set jim_handler_data, so remove it. Change-Id: Icc323faf754b0546a72208f90abd9e68ff2ef52f Signed-off-by: Antonio Borneo Reviewed-on: http://openocd.zylin.com/5667 Tested-by: jenkins Reviewed-by: Oleksij Rempel --- src/helper/command.c | 22 +++++------- src/helper/command.h | 71 +++++++++++++++++++++++++++++---------- src/target/arm_cti.c | 8 ++--- src/target/arm_dap.c | 8 ++--- src/target/arm_tpiu_swo.c | 6 +--- src/target/target.c | 6 +--- 6 files changed, 69 insertions(+), 52 deletions(-) diff --git a/src/helper/command.c b/src/helper/command.c index ecca75db6..89e217382 100644 --- a/src/helper/command.c +++ b/src/helper/command.c @@ -391,8 +391,9 @@ static struct command *register_command(struct command_context *context, return c; } -int register_commands(struct command_context *cmd_ctx, struct command *parent, - const struct command_registration *cmds) +int __register_commands(struct command_context *cmd_ctx, struct command *parent, + const struct command_registration *cmds, void *data, + struct target *override_target) { int retval = ERROR_OK; unsigned i; @@ -406,10 +407,12 @@ int register_commands(struct command_context *cmd_ctx, struct command *parent, retval = ERROR_FAIL; break; } + c->jim_handler_data = data; + c->jim_override_target = override_target; } if (NULL != cr->chain) { struct command *p = c ? : parent; - retval = register_commands(cmd_ctx, p, cr->chain); + retval = __register_commands(cmd_ctx, p, cr->chain, data, override_target); if (ERROR_OK != retval) break; } @@ -461,13 +464,6 @@ static int unregister_command(struct command_context *context, return ERROR_OK; } -void command_set_handler_data(struct command *c, void *p) -{ - c->jim_handler_data = p; - for (struct command *cc = c->children; NULL != cc; cc = cc->next) - command_set_handler_data(cc, p); -} - void command_output_text(struct command_context *context, const char *data) { if (context && context->output_handler && data) @@ -1057,12 +1053,12 @@ static int command_unknown(Jim_Interp *interp, int argc, Jim_Obj *const *argv) * 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; + if (c->jim_override_target) + cmd_ctx->current_target_override = c->jim_override_target; int retval = exec_command(interp, cmd_ctx, c, count, start); - if (c->jim_handler_data) + if (c->jim_override_target) cmd_ctx->current_target_override = saved_target_override; return retval; diff --git a/src/helper/command.h b/src/helper/command.h index cb088d743..871c064d3 100644 --- a/src/helper/command.h +++ b/src/helper/command.h @@ -186,11 +186,9 @@ struct command { command_handler_t handler; Jim_CmdProc *jim_handler; void *jim_handler_data; - /* Currently used only for target of target-prefixed cmd. - * Native OpenOCD commands use jim_handler_data exclusively - * as a target override. - * Jim handlers outside of target cmd tree can use - * jim_handler_data for any handler specific data */ + /* Command handlers can use it for any handler specific data */ + struct target *jim_override_target; + /* Used only for target of target-prefixed cmd */ enum command_mode mode; struct command *next; }; @@ -242,6 +240,10 @@ struct command_registration { /** Use this as the last entry in an array of command_registration records. */ #define COMMAND_REGISTRATION_DONE { .name = NULL, .chain = NULL } +int __register_commands(struct command_context *cmd_ctx, struct command *parent, + const struct command_registration *cmds, void *data, + struct target *override_target); + /** * Register one or more commands in the specified context, as children * of @c parent (or top-level commends, if NULL). In a registration's @@ -257,8 +259,53 @@ struct command_registration { * NULL for all fields. * @returns ERROR_OK on success; ERROR_FAIL if any registration fails. */ -int register_commands(struct command_context *cmd_ctx, struct command *parent, - const struct command_registration *cmds); +static inline int register_commands(struct command_context *cmd_ctx, struct command *parent, + const struct command_registration *cmds) +{ + return __register_commands(cmd_ctx, parent, cmds, NULL, NULL); +} + +/** + * Register one or more commands, as register_commands(), plus specify + * that command should override the current target + * + * @param cmd_ctx The command_context in which to register the command. + * @param parent Register this command as a child of this, or NULL to + * register a top-level command. + * @param cmds Pointer to an array of command_registration records that + * contains the desired command parameters. The last record must have + * NULL for all fields. + * @param target The target that has to override current target. + * @returns ERROR_OK on success; ERROR_FAIL if any registration fails. + */ +static inline int register_commands_override_target(struct command_context *cmd_ctx, + struct command *parent, const struct command_registration *cmds, + struct target *target) +{ + return __register_commands(cmd_ctx, parent, cmds, NULL, target); +} + +/** + * Register one or more commands, as register_commands(), plus specify + * a pointer to command private data that would be accessible through + * the macro CMD_DATA. The private data will not be freed when command + * is unregistered. + * + * @param cmd_ctx The command_context in which to register the command. + * @param parent Register this command as a child of this, or NULL to + * register a top-level command. + * @param cmds Pointer to an array of command_registration records that + * contains the desired command parameters. The last record must have + * NULL for all fields. + * @param data The command private data. + * @returns ERROR_OK on success; ERROR_FAIL if any registration fails. + */ +static inline int register_commands_with_data(struct command_context *cmd_ctx, + struct command *parent, const struct command_registration *cmds, + void *data) +{ + return __register_commands(cmd_ctx, parent, cmds, data, NULL); +} /** * Unregisters all commands from the specified context. @@ -272,16 +319,6 @@ int unregister_all_commands(struct command_context *cmd_ctx, struct command *command_find_in_context(struct command_context *cmd_ctx, const char *name); -/** - * Update the private command data field for a command and all descendents. - * This is used when creating a new hierarchy of commands that depends - * on obtaining a dynamically created context. The value will be available - * in command handlers by using the CMD_DATA macro. - * @param c The command (group) whose data pointer(s) will be updated. - * @param p The new data pointer to use for the command or its descendents. - */ -void command_set_handler_data(struct command *c, void *p); - void command_set_output_handler(struct command_context *context, command_output_handler_t output_handler, void *priv); diff --git a/src/target/arm_cti.c b/src/target/arm_cti.c index 689e9df9f..ee9d8aafd 100644 --- a/src/target/arm_cti.c +++ b/src/target/arm_cti.c @@ -507,17 +507,13 @@ static int cti_create(Jim_GetOptInfo *goi) }, COMMAND_REGISTRATION_DONE }; - e = register_commands(cmd_ctx, NULL, cti_commands); + e = register_commands_with_data(cmd_ctx, NULL, cti_commands, cti); if (ERROR_OK != e) return JIM_ERR; - struct command *c = command_find_in_context(cmd_ctx, cp); - assert(c); - command_set_handler_data(c, cti); - list_add_tail(&cti->lh, &all_cti); - return (ERROR_OK == e) ? JIM_OK : JIM_ERR; + return JIM_OK; } static int jim_cti_create(Jim_Interp *interp, int argc, Jim_Obj *const *argv) diff --git a/src/target/arm_dap.c b/src/target/arm_dap.c index 56442f183..a9277e798 100644 --- a/src/target/arm_dap.c +++ b/src/target/arm_dap.c @@ -265,17 +265,13 @@ static int dap_create(Jim_GetOptInfo *goi) if (transport_is_hla()) dap_commands[0].chain = NULL; - e = register_commands(cmd_ctx, NULL, dap_commands); + e = register_commands_with_data(cmd_ctx, NULL, dap_commands, dap); if (ERROR_OK != e) return JIM_ERR; - struct command *c = command_find_in_context(cmd_ctx, cp); - assert(c); - command_set_handler_data(c, dap); - list_add_tail(&dap->lh, &all_dap); - return (ERROR_OK == e) ? JIM_OK : JIM_ERR; + return JIM_OK; } static int jim_dap_create(Jim_Interp *interp, int argc, Jim_Obj *const *argv) diff --git a/src/target/arm_tpiu_swo.c b/src/target/arm_tpiu_swo.c index 543b4f008..186ce5d0e 100644 --- a/src/target/arm_tpiu_swo.c +++ b/src/target/arm_tpiu_swo.c @@ -886,14 +886,10 @@ static int arm_tpiu_swo_create(Jim_Interp *interp, struct arm_tpiu_swo_object *o }, COMMAND_REGISTRATION_DONE }; - e = register_commands(cmd_ctx, NULL, obj_commands); + e = register_commands_with_data(cmd_ctx, NULL, obj_commands, obj); if (ERROR_OK != e) return JIM_ERR; - struct command *c = command_find_in_context(cmd_ctx, obj->name); - assert(c); - command_set_handler_data(c, obj); - list_add_tail(&obj->lh, &all_tpiu_swo); return JIM_OK; diff --git a/src/target/target.c b/src/target/target.c index 619a8b490..fa033d351 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -5871,7 +5871,7 @@ static int target_create(Jim_GetOptInfo *goi) }, COMMAND_REGISTRATION_DONE }; - e = register_commands(cmd_ctx, NULL, target_commands); + e = register_commands_override_target(cmd_ctx, NULL, target_commands, target); if (e != ERROR_OK) { if (target->type->deinit_target) target->type->deinit_target(target); @@ -5884,10 +5884,6 @@ static int target_create(Jim_GetOptInfo *goi) return JIM_ERR; } - struct command *c = command_find_in_context(cmd_ctx, cp); - assert(c); - command_set_handler_data(c, target); - /* append to end of list */ append_to_list_all_targets(target);