| Message ID | 4F333B26.5050502@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 2012-02-09 04:19, Wen Congyang wrote: > Sync command needs these two APIs to suspend/resume monitor. > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > --- > 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. Jan
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 <wency@cn.fujitsu.com> >> --- >> 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. Thanks Wen Congyang > > Jan >
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 <wency@cn.fujitsu.com> >>> --- >>> 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. Jan
On Wed, 15 Feb 2012 09:51:04 +0100 Jan Kiszka <jan.kiszka@siemens.com> 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 <wency@cn.fujitsu.com> > >>> --- > >>> 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.
At 02/15/2012 09:01 PM, Luiz Capitulino Wrote: > On Wed, 15 Feb 2012 09:51:04 +0100 > Jan Kiszka <jan.kiszka@siemens.com> 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 <wency@cn.fujitsu.com> >>>>> --- >>>>> 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. I have read the newest migration code. I will change the code like that and remove these two APIs. Please ignore this patch Thanks Wen Congyang > > PS: Haven't reviewed this series yet. >
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,
Sync command needs these two APIs to suspend/resume monitor. Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- monitor.c | 27 +++++++++++++++++++++++++++ monitor.h | 2 ++ 2 files changed, 29 insertions(+), 0 deletions(-)