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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 25 May 2018 16:39:34 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, May 25, 2018 at 08:05:30AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >   
> > > On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:  
> > >> Markus Armbruster <armbru@redhat.com> writes:
> > >>   
> > >> > Igor Mammedov <imammedo@redhat.com> 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 <imammedo@redhat.com>
> > >> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> > >> >> ---
> > >> >> 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" <dgilbert@redhat.com>)
> > >> >> ---
> > >> >>  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.
I'll look into both options and try to come up a patch to make it better.