Well, drat. The patch that I pushed had some unintended consequences.
Any Jim subcommand handlers' output now gets suppressed too, which
negatively affected my recently posted series to split apart the
top-level Jim handlers.
While I am impacted severely, most everything that worked before my
refactoring should work again with only a couple of minor exceptions
(e.g. 'flash banks'). At the moment, any Jim-based subcommands will
appear to be broken; this can be overcome by prefixing the first command
verb with 'ocd_' (e.g. 'flash banks' works fine as 'ocd_flash banks').
This is a feeble temporary workaround, until I can put a more lasting
solution into place; however, there are few such handlers at present.
My proposed solution would be to update the "catcher" script to check
whether the ocd_ variant needs to be wrapped, which depends on the last
command verb. I am thinking about implementing this by introducing a
new 'command type' built-in, which would return 'jim' or 'handler' for a
given command invocation.
All 'handler' commands would continue to use the 'catch' block to
capture and consume the output, while 'jim' commands would pass their
output unmolested. The use of the 'command' verb paves the way to
support other useful introspective actions, but those features would
come in a subsequent patch series.
Incidentally, I am allowing for the future extension of a 'script' type,
which would allow calling another Jim function as a subcommand. Thus,
users might add their own subcommands in any part of the command tree,
using only Jim commands to do so. This also suggests a Jim event for
registering context-specific Jim commands (e.g. 'foo.cpu mycommand') or
updating the event system to define handlers as 'script' subcommands.
Thoughts?
--Z
On Fri, 2009-11-27 at 14:52 -0800, Zach Welch wrote:
> I just pushed a slightly improved version of the patch, which improves
> the fallback logic. It also uses the same trick for the help fallback,
> which was broken in the original version below.
>
> --Z
>
> On Fri, 2009-11-27 at 14:08 -0800, Zachary T Welch wrote:
> > The command refactoring caused subcommand handlers to produce duplicate
> > output when run. The problem was introduced by failing to ensure all
> > such invocations went through a top-level "catcher" script, prefixing
> > the command name with the 'ocd_' prefix and consuming its results.
> >
> > The fix is to ensure such a top-level "catcher" script gets created
> > for each top-level command, regardless of whether it has a handler.
> > Indeed, this patch removes all command registrations for sub-commands,
> > which would not have worked in the new registration scheme anyway.
> >
> > For now, dispatch of subcommands continues to be handled by the new
> > 'unknown' command handler, which gets fixed here to strip the 'ocd_'
> > prefix if searching for the top-level command name fails initially.
> > Some Jim commands may be registered with this prefix, and that situation
> > seems to require the current fallback approach. Otherwise, that prefix
> > could be stripped unconditionally and the logic made a little simpler.
> >
> > Overall, the command dispatching remains more complicated than desired,
> > but this patch fixes the immediate regressions.
> >
> > Signed-off-by: Zachary T Welch <z...@superlucidity.net>
> > ---
> > src/helper/command.c | 45 +++++++++++++++++++++++++++++++--------------
> > 1 files changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/helper/command.c b/src/helper/command.c
> > index 62fb487..135cd3f 100644
> > --- a/src/helper/command.c
> > +++ b/src/helper/command.c
> > @@ -44,6 +44,9 @@
> > #include "jim-eventloop.h"
> >
> >
> > +/* nice short description of source file */
> > +#define __THIS__FILE__ "command.c"
> > +
> > Jim_Interp *interp = NULL;
> >
> > static int run_command(struct command_context *context,
> > @@ -185,8 +188,12 @@ static int script_command(Jim_Interp *interp, int
> > argc, Jim_Obj *const *argv)
> > return script_command_run(interp, argc, argv, c, true);
> > }
> >
> > -/* nice short description of source file */
> > -#define __THIS__FILE__ "command.c"
> > +static struct command *command_root(struct command *c)
> > +{
> > + while (NULL != c->parent)
> > + c = c->parent;
> > + return c;
> > +}
> >
> > /**
> > * Find a command by name from a list of commands.
> > @@ -296,19 +303,22 @@ static int register_command_handler(struct command *c)
> > if (NULL == full_name)
> > return retval;
> >
> > - const char *ocd_name = alloc_printf("ocd_%s", full_name);
> > - if (NULL == full_name)
> > - goto free_full_name;
> > + if (NULL != c->handler)
> > + {
> > + const char *ocd_name = alloc_printf("ocd_%s", full_name);
> > + if (NULL == full_name)
> > + goto free_full_name;
> >
> > - Jim_CreateCommand(interp, ocd_name, script_command, c, NULL);
> > - free((void *)ocd_name);
> > + Jim_CreateCommand(interp, ocd_name, script_command, c, NULL);
> > + free((void *)ocd_name);
> > + }
> >
> > /* we now need to add an overrideable proc */
> > const char *override_name = alloc_printf("proc %s {args} {"
> > "if {[catch {eval ocd_%s $args}] == 0} "
> > "{return \"\"} else {return -code error}}",
> > full_name, full_name);
> > - if (NULL == full_name)
> > + if (NULL == override_name)
> > goto free_full_name;
> >
> > Jim_Eval_Named(interp, override_name, __THIS__FILE__, __LINE__);
> > @@ -343,7 +353,7 @@ struct command* register_command(struct command_context
> > *context,
> >
> > if (NULL != c->handler)
> > {
> > - int retval = register_command_handler(c);
> > + int retval = register_command_handler(command_root(c));
> > if (ERROR_OK != retval)
> > {
> > unregister_command(context, parent, name);
> > @@ -875,15 +885,22 @@ COMMAND_HANDLER(handle_help_command)
> > }
> >
> > static int command_unknown_find(unsigned argc, Jim_Obj *const *argv,
> > - struct command *head, struct command **out)
> > + struct command *head, struct command **out, bool top_level)
> > {
> > if (0 == argc)
> > return argc;
> > - struct command *c = command_find(head, Jim_GetString(argv[0], NULL));
> > + const char *cmd_name = Jim_GetString(argv[0], NULL);
> > + struct command *c = command_find(head, cmd_name);
> > if (NULL == c)
> > - return argc;
> > + {
> > + if (top_level && strncmp(cmd_name, "ocd_", 4) == 0)
> > + c = command_find(head, cmd_name + 4);
> > +
> > + if (NULL == c)
> > + return argc;
> > + }
> > *out = c;
> > - return command_unknown_find(--argc, ++argv, (*out)->children, out);
> > + return command_unknown_find(--argc, ++argv, (*out)->children, out,
> > false);
> > }
> >
> > static int command_unknown(Jim_Interp *interp, int argc, Jim_Obj *const
> > *argv)
> > @@ -893,7 +910,7 @@ static int command_unknown(Jim_Interp *interp, int
> > argc, Jim_Obj *const *argv)
> >
> > struct command_context *cmd_ctx = current_command_context();
> > struct command *c = cmd_ctx->commands;
> > - int remaining = command_unknown_find(argc - 1, argv + 1, c, &c);
> > + int remaining = command_unknown_find(argc - 1, argv + 1, c, &c, true);
> > // if nothing could be consumed, then it's really an unknown command
> > if (remaining == argc - 1)
> > {
>
>
> _______________________________________________
> Openocd-development mailing list
> Openocd-development@lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/openocd-development
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
