Patchwork [5/9] Monitor: Return before exiting with 'quit'

login
register
mail settings
Submitter Luiz Capitulino
Date April 26, 2010, 6:53 p.m.
Message ID <20100426155303.2c381ff7@redhat.com>
Download mbox | patch
Permalink /patch/50998/
State New
Headers show

Comments

Luiz Capitulino - April 26, 2010, 6:53 p.m.
On Mon, 26 Apr 2010 13:25:38 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 04/26/2010 01:22 PM, Luiz Capitulino wrote:
> > On Mon, 26 Apr 2010 12:49:40 -0500
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >    
> >> On 04/26/2010 10:47 AM, Luiz Capitulino wrote:
> >>      
> >>> The 'quit' Monitor command (implemented by do_quit()) calls
> >>> exit() directly, this is problematic under QMP because QEMU
> >>> exits before having a chance to send the ok response.
> >>>
> >>> Clients don't know if QEMU exited because of a problem or
> >>> because the 'quit' command has been executed.
> >>>
> >>> This commit fixes that by moving the exit() call to the main
> >>> loop, so that do_quit() requests the system to quit, instead
> >>> of calling exit() directly.
> >>>
> >>>        
> >> Does this also have the effect of printing out a (qemu) prompt after
> >> quit before an EOF appears on that socket?
> >>      
> >   Ah, right..
> >    
> 
> It's not necessarily a bad thing if it does.  I just wanted to raise 
> that because it's possible that someone depends on the behavior.

 Yes, and it's also a bit ugly if you do '-monitor stdio':

~/stuff/virt/ ./qemu-qmp -monitor stdio
QEMU 0.12.50 monitor - type 'help' for more information
(qemu) quit
(qemu) ~/stuff/virt/

 So, I'd like to fix it.

> I'm not sure it matters to me if we change this behavior though.

 It's very easy to fix, the following patch does it.

 I have (tested and) rebased the for-anthony branch with it.

From 14ecef47d1989f9b646fc0fe7a4bf42c091d1432 Mon Sep 17 00:00:00 2001
From: Luiz Capitulino <lcapitulino@redhat.com>
Date: Tue, 6 Apr 2010 18:55:54 -0300
Subject: [PATCH 5/9] Monitor: Return before exiting with 'quit'

The 'quit' Monitor command (implemented by do_quit()) calls
exit() directly, this is problematic under QMP because QEMU
exits before having a chance to send the ok response.

Clients don't know if QEMU exited because of a problem or
because the 'quit' command has been executed.

This commit fixes that by moving the exit() call to the main
loop, so that do_quit() requests the system to quit, instead
of calling exit() directly.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    7 ++++++-
 sysemu.h  |    2 ++
 vl.c      |   18 ++++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletions(-)
Anthony Liguori - April 26, 2010, 7 p.m.
On 04/26/2010 01:53 PM, Luiz Capitulino wrote:
> On Mon, 26 Apr 2010 13:25:38 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 04/26/2010 01:22 PM, Luiz Capitulino wrote:
>>      
>>> On Mon, 26 Apr 2010 12:49:40 -0500
>>> Anthony Liguori<anthony@codemonkey.ws>   wrote:
>>>
>>>
>>>        
>>>> On 04/26/2010 10:47 AM, Luiz Capitulino wrote:
>>>>
>>>>          
>>>>> The 'quit' Monitor command (implemented by do_quit()) calls
>>>>> exit() directly, this is problematic under QMP because QEMU
>>>>> exits before having a chance to send the ok response.
>>>>>
>>>>> Clients don't know if QEMU exited because of a problem or
>>>>> because the 'quit' command has been executed.
>>>>>
>>>>> This commit fixes that by moving the exit() call to the main
>>>>> loop, so that do_quit() requests the system to quit, instead
>>>>> of calling exit() directly.
>>>>>
>>>>>
>>>>>            
>>>> Does this also have the effect of printing out a (qemu) prompt after
>>>> quit before an EOF appears on that socket?
>>>>
>>>>          
>>>    Ah, right..
>>>
>>>        
>> It's not necessarily a bad thing if it does.  I just wanted to raise
>> that because it's possible that someone depends on the behavior.
>>      
>   Yes, and it's also a bit ugly if you do '-monitor stdio':
>
> ~/stuff/virt/ ./qemu-qmp -monitor stdio
> QEMU 0.12.50 monitor - type 'help' for more information
> (qemu) quit
> (qemu) ~/stuff/virt/
>
>   So, I'd like to fix it.
>
>    
>> I'm not sure it matters to me if we change this behavior though.
>>      
>   It's very easy to fix, the following patch does it.
>
>   I have (tested and) rebased the for-anthony branch with it.
>
>  From 14ecef47d1989f9b646fc0fe7a4bf42c091d1432 Mon Sep 17 00:00:00 2001
> From: Luiz Capitulino<lcapitulino@redhat.com>
> Date: Tue, 6 Apr 2010 18:55:54 -0300
> Subject: [PATCH 5/9] Monitor: Return before exiting with 'quit'
>
> The 'quit' Monitor command (implemented by do_quit()) calls
> exit() directly, this is problematic under QMP because QEMU
> exits before having a chance to send the ok response.
>
> Clients don't know if QEMU exited because of a problem or
> because the 'quit' command has been executed.
>
> This commit fixes that by moving the exit() call to the main
> loop, so that do_quit() requests the system to quit, instead
> of calling exit() directly.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   monitor.c |    7 ++++++-
>   sysemu.h  |    2 ++
>   vl.c      |   18 ++++++++++++++++++
>   3 files changed, 26 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index ef84298..5b0258f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1017,7 +1017,12 @@ static void do_info_cpu_stats(Monitor *mon)
>    */
>   static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
> -    exit(0);
> +    if (monitor_ctrl_mode(mon)) {
> +        qemu_system_exit_request();
> +    } else {
> +        exit(0);
> +    }
> +
>    

Now they have different behaviors though because 
qemu_system_exit_request() actually exits gracefully.

For instance, now in QMP mode, an ifdown script will be executed when 
the 'quit' command is invoked whereas it won't be executed when done 
through the human monitor.

I honestly don't mind the weird print outs with -monitor stdio.  I think 
exiting gracefully in both cases is better.

Regards,

Anthony Liguori

>       return 0;
>   }
>
> diff --git a/sysemu.h b/sysemu.h
> index d0effa0..fa921df 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -45,9 +45,11 @@ void cpu_disable_ticks(void);
>   void qemu_system_reset_request(void);
>   void qemu_system_shutdown_request(void);
>   void qemu_system_powerdown_request(void);
> +void qemu_system_exit_request(void);
>   int qemu_shutdown_requested(void);
>   int qemu_reset_requested(void);
>   int qemu_powerdown_requested(void);
> +int qemu_exit_requested(void);
>   extern qemu_irq qemu_system_powerdown;
>   void qemu_system_reset(void);
>
> diff --git a/vl.c b/vl.c
> index a5a0f41..9ef6f2c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1697,6 +1697,7 @@ static int shutdown_requested;
>   static int powerdown_requested;
>   int debug_requested;
>   int vmstop_requested;
> +static int exit_requested;
>
>   int qemu_shutdown_requested(void)
>   {
> @@ -1719,6 +1720,12 @@ int qemu_powerdown_requested(void)
>       return r;
>   }
>
> +int qemu_exit_requested(void)
> +{
> +    /* just return it, we'll exit() anyway */
> +    return exit_requested;
> +}
> +
>   static int qemu_debug_requested(void)
>   {
>       int r = debug_requested;
> @@ -1789,6 +1796,12 @@ void qemu_system_powerdown_request(void)
>       qemu_notify_event();
>   }
>
> +void qemu_system_exit_request(void)
> +{
> +    exit_requested = 1;
> +    qemu_notify_event();
> +}
> +
>   #ifdef _WIN32
>   static void host_main_loop_wait(int *timeout)
>   {
> @@ -1925,6 +1938,8 @@ static int vm_can_run(void)
>           return 0;
>       if (debug_requested)
>           return 0;
> +    if (exit_requested)
> +        return 0;
>       return 1;
>   }
>
> @@ -1977,6 +1992,9 @@ static void main_loop(void)
>           if ((r = qemu_vmstop_requested())) {
>               vm_stop(r);
>           }
> +        if (qemu_exit_requested()) {
> +            exit(0);
> +        }
>       }
>       pause_all_vcpus();
>   }
>
Jan Kiszka - April 26, 2010, 7:10 p.m.
Anthony Liguori wrote:
> On 04/26/2010 01:53 PM, Luiz Capitulino wrote:
>> On Mon, 26 Apr 2010 13:25:38 -0500
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>
>>   
>>> On 04/26/2010 01:22 PM, Luiz Capitulino wrote:
>>>     
>>>> On Mon, 26 Apr 2010 12:49:40 -0500
>>>> Anthony Liguori<anthony@codemonkey.ws>   wrote:
>>>>
>>>>
>>>>       
>>>>> On 04/26/2010 10:47 AM, Luiz Capitulino wrote:
>>>>>
>>>>>         
>>>>>> The 'quit' Monitor command (implemented by do_quit()) calls
>>>>>> exit() directly, this is problematic under QMP because QEMU
>>>>>> exits before having a chance to send the ok response.
>>>>>>
>>>>>> Clients don't know if QEMU exited because of a problem or
>>>>>> because the 'quit' command has been executed.
>>>>>>
>>>>>> This commit fixes that by moving the exit() call to the main
>>>>>> loop, so that do_quit() requests the system to quit, instead
>>>>>> of calling exit() directly.
>>>>>>
>>>>>>
>>>>>>            
>>>>> Does this also have the effect of printing out a (qemu) prompt after
>>>>> quit before an EOF appears on that socket?
>>>>>
>>>>>          
>>>>    Ah, right..
>>>>
>>>>        
>>> It's not necessarily a bad thing if it does.  I just wanted to raise
>>> that because it's possible that someone depends on the behavior.
>>>      
>>   Yes, and it's also a bit ugly if you do '-monitor stdio':
>>
>> ~/stuff/virt/ ./qemu-qmp -monitor stdio
>> QEMU 0.12.50 monitor - type 'help' for more information
>> (qemu) quit
>> (qemu) ~/stuff/virt/
>>
>>   So, I'd like to fix it.
>>
>>   
>>> I'm not sure it matters to me if we change this behavior though.
>>>      
>>   It's very easy to fix, the following patch does it.
>>
>>   I have (tested and) rebased the for-anthony branch with it.
>>
>>  From 14ecef47d1989f9b646fc0fe7a4bf42c091d1432 Mon Sep 17 00:00:00 2001
>> From: Luiz Capitulino<lcapitulino@redhat.com>
>> Date: Tue, 6 Apr 2010 18:55:54 -0300
>> Subject: [PATCH 5/9] Monitor: Return before exiting with 'quit'
>>
>> The 'quit' Monitor command (implemented by do_quit()) calls
>> exit() directly, this is problematic under QMP because QEMU
>> exits before having a chance to send the ok response.
>>
>> Clients don't know if QEMU exited because of a problem or
>> because the 'quit' command has been executed.
>>
>> This commit fixes that by moving the exit() call to the main
>> loop, so that do_quit() requests the system to quit, instead
>> of calling exit() directly.
>>
>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>> ---
>>   monitor.c |    7 ++++++-
>>   sysemu.h  |    2 ++
>>   vl.c      |   18 ++++++++++++++++++
>>   3 files changed, 26 insertions(+), 1 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index ef84298..5b0258f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1017,7 +1017,12 @@ static void do_info_cpu_stats(Monitor *mon)
>>    */
>>   static int do_quit(Monitor *mon, const QDict *qdict, QObject
>> **ret_data)
>>   {
>> -    exit(0);
>> +    if (monitor_ctrl_mode(mon)) {
>> +        qemu_system_exit_request();
>> +    } else {
>> +        exit(0);
>> +    }
>> +
>>    
> 
> Now they have different behaviors though because
> qemu_system_exit_request() actually exits gracefully.
> 
> For instance, now in QMP mode, an ifdown script will be executed when
> the 'quit' command is invoked whereas it won't be executed when done
> through the human monitor.
> 
> I honestly don't mind the weird print outs with -monitor stdio.  I think
> exiting gracefully in both cases is better.

Just suspend the monitor before leaving for termination. That should
have both the proper visual and functional effect.

Jan
Anthony Liguori - April 26, 2010, 7:13 p.m.
On 04/26/2010 02:10 PM, Jan Kiszka wrote:
>
> Just suspend the monitor before leaving for termination. That should
> have both the proper visual and functional effect.
>    

Very good idea.

Regards,

Anthony Liguori

> Jan
>
>
>

Patch

diff --git a/monitor.c b/monitor.c
index ef84298..5b0258f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1017,7 +1017,12 @@  static void do_info_cpu_stats(Monitor *mon)
  */
 static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    exit(0);
+    if (monitor_ctrl_mode(mon)) {
+        qemu_system_exit_request();
+    } else {
+        exit(0);
+    }
+
     return 0;
 }
 
diff --git a/sysemu.h b/sysemu.h
index d0effa0..fa921df 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -45,9 +45,11 @@  void cpu_disable_ticks(void);
 void qemu_system_reset_request(void);
 void qemu_system_shutdown_request(void);
 void qemu_system_powerdown_request(void);
+void qemu_system_exit_request(void);
 int qemu_shutdown_requested(void);
 int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
+int qemu_exit_requested(void);
 extern qemu_irq qemu_system_powerdown;
 void qemu_system_reset(void);
 
diff --git a/vl.c b/vl.c
index a5a0f41..9ef6f2c 100644
--- a/vl.c
+++ b/vl.c
@@ -1697,6 +1697,7 @@  static int shutdown_requested;
 static int powerdown_requested;
 int debug_requested;
 int vmstop_requested;
+static int exit_requested;
 
 int qemu_shutdown_requested(void)
 {
@@ -1719,6 +1720,12 @@  int qemu_powerdown_requested(void)
     return r;
 }
 
+int qemu_exit_requested(void)
+{
+    /* just return it, we'll exit() anyway */
+    return exit_requested;
+}
+
 static int qemu_debug_requested(void)
 {
     int r = debug_requested;
@@ -1789,6 +1796,12 @@  void qemu_system_powerdown_request(void)
     qemu_notify_event();
 }
 
+void qemu_system_exit_request(void)
+{
+    exit_requested = 1;
+    qemu_notify_event();
+}
+
 #ifdef _WIN32
 static void host_main_loop_wait(int *timeout)
 {
@@ -1925,6 +1938,8 @@  static int vm_can_run(void)
         return 0;
     if (debug_requested)
         return 0;
+    if (exit_requested)
+        return 0;
     return 1;
 }
 
@@ -1977,6 +1992,9 @@  static void main_loop(void)
         if ((r = qemu_vmstop_requested())) {
             vm_stop(r);
         }
+        if (qemu_exit_requested()) {
+            exit(0);
+        }
     }
     pause_all_vcpus();
 }