Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
> 
> > * Markus Armbruster (address@hidden) wrote:
> >> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <address@hidden>
> >> >
> >> > Allow the 'help' command in preconfig state but
> >> > make it only list the preconfig commands.
> >> >
> >> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> >> > Reviewed-by: Peter Xu <address@hidden>
> >> > Reviewed-by: Igor Mammedov <address@hidden>
> >> > ---
> >> >  hmp-commands.hx | 1 +
> >> >  monitor.c       | 8 +++++++-
> >> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> > index 0734fea931..8bf590ae4b 100644
> >> > --- a/hmp-commands.hx
> >> > +++ b/hmp-commands.hx
> >> > @@ -15,6 +15,7 @@ ETEXI
> >> >          .params     = "[cmd]",
> >> >          .help       = "show the help",
> >> >          .cmd        = do_help_cmd,
> >> > +        .flags      = "p",
> >> >      },
> >> >  
> >> >  STEXI
> >> > diff --git a/monitor.c b/monitor.c
> >> > index f4a16e6a03..31c8f5dc88 100644
> >> > --- a/monitor.c
> >> > +++ b/monitor.c
> >> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
> >> >  {
> >> >      int i;
> >> >  
> >> > +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) 
> >> > {
> >> > +        return;
> >> > +    }
> >> > +
> >> >      for (i = 0; i < prefix_args_nb; i++) {
> >> >          monitor_printf(mon, "%s ", prefix_args[i]);
> >> >      }
> >> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const 
> >> > mon_cmd_t *cmds,
> >> >  
> >> >      /* Find one entry to dump */
> >> >      for (cmd = cmds; cmd->name != NULL; cmd++) {
> >> > -        if (compare_cmd(args[arg_index], cmd->name)) {
> >> > +        if (compare_cmd(args[arg_index], cmd->name) &&
> >> > +            ((!runstate_check(RUN_STATE_PRECONFIG) ||
> >> > +                cmd_can_preconfig(cmd)))) {
> >> 
> >> Why do we need this check in addition to the one in help_cmd_dump_one()?
> >
> > To gate entire subtables (we've only currently got 'info' and that's 
> > enabled,
> > anyway, so it never actually triggers).
> 
> Something's bothering me around here, but I can't put a finger on it...
> let me think aloud.
> 
> There's an asymmetry between command execution and help.  The former has
> just one check, in monitor_parse_command().  If the command has a
> sub-table, and there are arguments, monitor_parse_command() recurses
> into the sub-table.  Thus, if the command is unavailable, the
> sub-commands are unavailable as well, regardless of their "p" flags.
> 
> Help's recursion is different.  It's limited to two levels, unlike
> execution.  help_cmd_dump_one()'s check applies to the last level.
> help_cmd_dump()'s check applies to the first level.  If there's just one
> level, we check twice.  Perhaps less than elegant, but harmless and
> simple.
> 
> You can't make a sub-command available without also making the command
> available.  Makes sense, I guess.  If you try, your "p" flags on the
> sub-commands are silently ignored.  That's a bit ugly, but if it doesn't
> bother you, I can pretend I didn't see it ;)

Yes I'm OK with that; you've got to enable all the way down to the
command that youw ant to enable.

I think the difference between execution and help comes from the
way you can do 'help' on it's own to list everything, and help
on a subtable (help info) and help on a command (help info uuid);
I think that's why the recursion gets a bit hairier.

Dave

> > Dave
> >
> >> >              if (cmd->sub_table) {
> >> >                  /* continue with next arg */
> >> >                  help_cmd_dump(mon, cmd->sub_table,
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK