[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: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state |
Date: |
Fri, 25 May 2018 16:39:34 -0300 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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
- [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