Patchwork [RFC,01/16,v6] monitor: introduce qemu_suspend_monitor()/qemu_resume_monitor()

login
register
mail settings
Submitter Wen Congyang
Date Feb. 9, 2012, 3:19 a.m.
Message ID <4F333B26.5050502@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/140291/
State New
Headers show

Comments

Wen Congyang - Feb. 9, 2012, 3:19 a.m.
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(-)
Jan Kiszka - Feb. 14, 2012, 4:19 p.m.
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
Wen Congyang - Feb. 15, 2012, 2:54 a.m.
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
>
Jan Kiszka - Feb. 15, 2012, 8:51 a.m.
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
Luiz Capitulino - Feb. 15, 2012, 1:01 p.m.
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.
Wen Congyang - Feb. 16, 2012, 1:35 a.m.
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.
>

Patch

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,