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 <pet...@redhat.com>
> > > > > ---
> > > > > 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.
Stefan
signature.asc
Description: PGP signature
