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

[Peter Xu]

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.

Thanks,

-- 
Peter Xu