Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state

[Eduardo Habkost]

On Fri, May 25, 2018 at 08:05:30AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
> 
> > On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:
> >> Markus Armbruster <address@hidden> writes:
> >> 
> >> > Igor Mammedov <address@hidden> writes:
> >> >
> >> >> Ban it for now, if someone would need it to work early,
> >> >> one would have to implement checks if HMP command is valid
> >> >> at preconfig state.
> >> >>
> >> >> Signed-off-by: Igor Mammedov <address@hidden>
> >> >> Reviewed-by: Eric Blake <address@hidden>
> >> >> ---
> >> >> v5:
> >> >>   * add 'use QMP instead" to error message, suggesting user
> >> >>     the right interface to use
> >> >> v4:
> >> >>   * v3 was only printing error but not preventing command execution,
> >> >>     Fix it by returning after printing error message.
> >> >>     ("Dr. David Alan Gilbert" <address@hidden>)
> >> >> ---
> >> >>  monitor.c | 6 ++++++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/monitor.c b/monitor.c
> >> >> index 39f8ee1..0ffdf1d 100644
> >> >> --- a/monitor.c
> >> >> +++ b/monitor.c
> >> >> @@ -3374,6 +3374,12 @@ static void handle_hmp_command(Monitor *mon, 
> >> >> const char *cmdline)
> >> >>  
> >> >>      trace_handle_hmp_command(mon, cmdline);
> >> >>  
> >> >> +    if (runstate_check(RUN_STATE_PRECONFIG)) {
> >> >> +        monitor_printf(mon, "HMP not available in preconfig state, "
> >> >> +                            "use QMP instead\n");
> >> >> +        return;
> >> >> +    }
> >> >> +
> >> >>      cmd = monitor_parse_command(mon, cmdline, &cmdline, 
> >> >> mon->cmd_table);
> >> >>      if (!cmd) {
> >> >>          return;
> >> >
> >> > So we offer the user an HMP monitor, but we summarily fail all commands.
> >> > I'm sorry, but that's... searching for polite word... embarrassing.  We
> >> > should accept HMP output only when we're ready to accept it.  Yes, that
> >> > would involve a bit more surgery rather than this cheap hack.  The whole
> >> > preconfig thing smells like a cheap hack to me, but let's not overdo it.
> >> 
> >> Clarification: I don't think we need to hold the series because of
> >> this.  I do think you should investigate delaying HMP until it can work.
> >
> > What would it mean to delay HMP?  Not creating the socket?
> > Creating the socket but not accepting clients?  Accepting clients
> > but not consuming any input from the socket until we are out of
> > preconfig?
> >
> > I'm not sure if any of those options would be better.  If a human
> > is already trying to talk on the other side, it seems better to
> > show QEMU is alive (but not ready to hold a conversation yet)
> > than staying silent.
> 
> If this
> 
>     QEMU 2.12.50 monitor - type 'help' for more information
>     (qemu) help
>     HMP not available in preconfig state, use QMP instead
>     (qemu) quit
>     HMP not available in preconfig state, use QMP instead
>     (qemu) let me out dammit
>     HMP not available in preconfig state, use QMP instead
>     (qemu) 
> 
> is better than the alternatives, then I wonder how much more
> entertainment the alternatives could provide!
> 
> We *can* do better.  Start like this:
> 
>     QEMU 2.12.50 monitor is not ready with -preconfig until you complete
>     configuration with QMP
> 
> and when we exit preconfig state, add:
> 
>     QEMU 2.12.50 monitor - type 'help' for more information
>     (qemu) 
> 
> Note that this is upfront about the monitor not being ready, avoids
> misleading the user about "help", talks to the user in the user's terms
> (-preconfig) instead of internal terms (preconfig state), and is more
> specific on how to ready the monitor.

Yes, this sounds better than any of the options I have
considered.

Making at least 'help', 'quit', and 'exit-preconfig' work might
be even better, though.

-- 
Eduardo