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

[Dr. David Alan Gilbert]

* Igor Mammedov (address@hidden) wrote:
> On Fri, 25 May 2018 16:39:34 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > 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.
> I'll look into both options and try to come up a patch to make it better.

Lets keep whatever we do here simple.
As I understand it, the only reason to deny HMP in preconfig is
because we've not got a per-command flag to say which commands
are allowed in preconfig state.  If you're going to allow
'help', 'quit' etc then you just end up adding that flag
(which should be easy) and then we've got the flag and
we can go back and enable other HMP commands in preconfig as well.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK