Re: [Qemu-devel] [RFC][PATCH 01/16 v6] monitor: introduce qemu_suspend_monitor()/qemu_resume_monitor()

[Luiz Capitulino]

On Wed, 15 Feb 2012 09:51:04 +0100
Jan Kiszka <address@hidden> wrote:

> On 2012-02-15 03:54, Wen Congyang wrote:
> > At 02/15/2012 12:19 AM, Jan Kiszka Wrote:
> >> On 2012-02-09 04:19, Wen Congyang wrote:
> >>> Sync command needs these two APIs to suspend/resume monitor.
> >>>
> >>> Signed-off-by: Wen Congyang <address@hidden>
> >>> ---
> >>>  monitor.c |   27 +++++++++++++++++++++++++++
> >>>  monitor.h |    2 ++
> >>>  2 files changed, 29 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/monitor.c b/monitor.c
> >>> index 11639b1..7e72739 100644
> >>> --- a/monitor.c
> >>> +++ b/monitor.c
> >>> @@ -4442,6 +4442,26 @@ static void monitor_command_cb(Monitor *mon, const 
> >>> char *cmdline, void *opaque)
> >>>      monitor_resume(mon);
> >>>  }
> >>>  
> >>> +int qemu_suspend_monitor(const char *fmt, ...)
> >>> +{
> >>> +    int ret;
> >>> +
> >>> +    if (cur_mon) {
> >>> +        ret = monitor_suspend(cur_mon);
> >>> +    } else {
> >>> +        ret = -ENOTTY;
> >>> +    }
> >>> +
> >>> +    if (ret < 0 && fmt) {
> >>> +        va_list ap;
> >>> +        va_start(ap, fmt);
> >>> +        monitor_vprintf(cur_mon, fmt, ap);
> >>> +        va_end(ap);
> >>> +    }
> >>> +
> >>> +    return ret;
> >>> +}
> >>> +
> >>>  int monitor_suspend(Monitor *mon)
> >>>  {
> >>>      if (!mon->rs)
> >>> @@ -4450,6 +4470,13 @@ int monitor_suspend(Monitor *mon)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +void qemu_resume_monitor(void)
> >>> +{
> >>> +    if (cur_mon) {
> >>> +        monitor_resume(cur_mon);
> >>> +    }
> >>> +}
> >>> +
> >>>  void monitor_resume(Monitor *mon)
> >>>  {
> >>>      if (!mon->rs)
> >>> diff --git a/monitor.h b/monitor.h
> >>> index 58109af..60a1e17 100644
> >>> --- a/monitor.h
> >>> +++ b/monitor.h
> >>> @@ -46,7 +46,9 @@ int monitor_cur_is_qmp(void);
> >>>  void monitor_protocol_event(MonitorEvent event, QObject *data);
> >>>  void monitor_init(CharDriverState *chr, int flags);
> >>>  
> >>> +int qemu_suspend_monitor(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> >>>  int monitor_suspend(Monitor *mon);
> >>> +void qemu_resume_monitor(void);
> >>>  void monitor_resume(Monitor *mon);
> >>>  
> >>>  int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
> >>
> >> I don't see any added value in this API, specifically as it is built on
> >> top of cur_mon. Just use the existing services like the migration code
> >> does. If you properly pass down the monitor reference from the command
> >> to the suspend and store what monitor you suspended, all should be fine.
> > 
> > This API is like qemu_get_fd() which is not merged into upstream qemu.
> > I need this API because I cannot use monitor in qapi command.
> 
> OK, then I need to comment on that approach. QMP looks flawed here.
> Either you have a need for a Monitor object (or a generic HMP/QMP
> context), then you also have a handle. Or your don't, then you do not
> need monitor suspend/resume or get_fd as well.

The getfd one is explained in the other thread, but suspend/resume should
be done from HMP only.

PS: Haven't reviewed this series yet.