Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Peter Xu]

On Fri, Jan 12, 2018 at 02:28:32PM +0000, Stefan Hajnoczi wrote:
> On Fri, Jan 12, 2018 at 12:51:49PM +0800, Peter Xu wrote:
> > On Mon, Jan 08, 2018 at 04:49:36PM +0000, Stefan Hajnoczi wrote:
> > > On Mon, Dec 25, 2017 at 11:26:13AM +0800, Peter Xu wrote:
> > > > On Thu, Dec 21, 2017 at 07:27:38PM +0800, Fam Zheng wrote:
> > > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > > One thing to mention is that for QMPs that are using IOThreads, we 
> > > > > > need
> > > > > > an explicit kick for the IOThread in case it is sleeping.
> > > > > > 
> > > > > > Since at it, add traces for the operations.
> > > > > > 
> > > > > > Signed-off-by: Peter Xu <address@hidden>
> > > > > > ---
> > > > > >  monitor.c    | 26 +++++++++++++++++++++-----
> > > > > >  trace-events |  1 +
> > > > > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/monitor.c b/monitor.c
> > > > > > index 844508d134..5f05f2e9da 100644
> > > > > > --- a/monitor.c
> > > > > > +++ b/monitor.c
> > > > > > @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void 
> > > > > > *opaque, const char *cmdline,
> > > > > >  
> > > > > >  int monitor_suspend(Monitor *mon)
> > > > > >  {
> > > > > > -    if (!mon->rs)
> > > > > > -        return -ENOTTY;
> > > > > 
> > > > > Please add to the commit message why the mon->rs check is dropped, 
> > > > > for this and
> > > > > the other one.
> > > > 
> > > > I thought it would be clear enough since mon->rs is only used by HMP
> > > > and the subject tells us that this patch is adding support for QMP.
> > > > But... sure I can add one more sentence for that!
> > > 
> > > This change breaks hmp.c:hmp_migrate():
> > > 
> > >   if (monitor_suspend(mon) < 0) {
> > >       monitor_printf(mon, "terminal does not allow synchronous "
> > >                      "migration, continuing detached\n");
> > >       return;
> > >   }
> > > 
> > > mon->rs is used by HMP code to determine whether this is an interactive
> > > terminal.  Both live migration and password prompting rely on this
> > > because they must be forbidden when doing so would be impossible (e.g.
> > > GDB stub tunneling HMP commands).
> > > 
> > > Suspending the QMP monitor is useful, but please don't break the HMP
> > > code.
> > 
> > Could you elaborate a bit on how this patch breaks anything?
> > 
> > IMHO HMP monitors always have Monitor.rs setup, so here in
> > hmp_migrate() I don't really understand why we need this check, since
> > IIUC monitor_suspend() for a HMP monitor will never fail before.
> 
> No, HMP monitors do not always have ->rs.  I mentioned that the GDB stub
> uses a non-interactive HMP monitor.
> 
> Here is the code from gdbstub.c:
> 
>   /* Initialize a monitor terminal for gdb */
>   mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
>                              NULL, &error_abort);
>   monitor_init(mon_chr, 0);
> 
> flags=0 means !MONITOR_USE_CONTROL (HMP) and !MONITOR_USE_READLINE
> (non-interactive).
> 
> Your patch breaks this feature.

Ah I see the point.  I'll fix that up.  So Fam's questioning is valid,
mon->rs needs to be checked.

Even, I would prefer to check the flags - they are more understandable
than checking against mon->rs.

Thanks for explaining.

-- 
Peter Xu