"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
>>
>> > * Markus Armbruster (arm...@redhat.com) wrote:
>> >> "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> writes:
>> >>
>> >> > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>> >> >
>> >> > Allow the 'help' command in preconfig state but
>> >> > make it only list the preconfig commands.
>> >> >
>> >> > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
>> >> > Reviewed-by: Peter Xu <pet...@redhat.com>
>> >> > Reviewed-by: Igor Mammedov <imamm...@redhat.com>
>> >> > ---
>> >> > 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.
Well, you can execute the command on its own (info), and a sub-command
(info uuid) --- same structure. Anyway, what we have works.
