[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig stat
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state |
Date: |
Mon, 28 May 2018 11:31:30 +0200 |
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.
- Re: [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options(), (continued)
[Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig, Igor Mammedov, 2018/05/04
[Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig", Igor Mammedov, 2018/05/04
[Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option, Igor Mammedov, 2018/05/04