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

login
register
mail settings
Submitter Luiz Capitulino
Date April 26, 2010, 3:47 p.m.
Message ID <1272296853-30285-6-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/50975/
State New
Headers show

Comments

Luiz Capitulino - April 26, 2010, 3:47 p.m.
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 |    2 +-
 sysemu.h  |    2 ++
 vl.c      |   18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 1 deletions(-)
Anthony Liguori - April 26, 2010, 5:49 p.m.
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?

Regards,

Anthony Liguori

> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   monitor.c |    2 +-
>   sysemu.h  |    2 ++
>   vl.c      |   18 ++++++++++++++++++
>   3 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index ef84298..611cbe9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1017,7 +1017,7 @@ static void do_info_cpu_stats(Monitor *mon)
>    */
>   static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
> -    exit(0);
> +    qemu_system_exit_request();
>       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();
>   }
>
Luiz Capitulino - April 26, 2010, 6:22 p.m.
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..

 So, the easiest way to fix this is:

if (user monitor) {
   exit(0);
} else {
   go through main;
}

 And, wrt to the pull, assuming you like the other patches, what's the best for you?

 Should I just drop this patch and ask you to pull again or can I do the fix,
rebase, send it in this thread, and ping you?
Anthony Liguori - April 26, 2010, 6:25 p.m.
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.

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

>   So, the easiest way to fix this is:
>
> if (user monitor) {
>     exit(0);
> } else {
>     go through main;
> }
>
>   And, wrt to the pull, assuming you like the other patches, what's the best for you?
>    

I'm happy to pull it, just wanted to see fi this issue was considered 
before I did.

Regards,

Anthony Liguori

>   Should I just drop this patch and ask you to pull again or can I do the fix,
> rebase, send it in this thread, and ping you?
>

Patch

diff --git a/monitor.c b/monitor.c
index ef84298..611cbe9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1017,7 +1017,7 @@  static void do_info_cpu_stats(Monitor *mon)
  */
 static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    exit(0);
+    qemu_system_exit_request();
     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();
 }