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

login
register
mail settings
Submitter Luiz Capitulino
Date April 26, 2010, 7:44 p.m.
Message ID <20100426164448.679a4d19@redhat.com>
Download mbox | patch
Permalink /patch/50999/
State New
Headers show

Comments

Luiz Capitulino - April 26, 2010, 7:44 p.m.
On Mon, 26 Apr 2010 14:13:40 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> 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.

 Yeah, works great.

 Final version of the patch below, for-anthony branch updated.

From 0e8d2b5575938b8876a3c4bb66ee13c5d306fb6d 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 |    3 ++-
 sysemu.h  |    2 ++
 vl.c      |   18 ++++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletions(-)
Paolo Bonzini - April 27, 2010, 11:52 a.m.
On 04/26/2010 09:44 PM, Luiz Capitulino wrote:
> +    qemu_system_exit_request();

Untested suggestion: why add qemu_system_exit_request, exit_requested, 
and a hook in the main loop?  You can do instead

    no_shutdown = 0;
    qemu_system_shutdown_request();

which will actually call quit_timers() and net_cleanup() properly unlike 
a blind exit(0).

Alternatively, just give an error when "quit"-ting from QMP and keep the 
current behavior for non-QMP.  This way you do not provide two ways to 
do the same thing.  People will have to avoid -no-shutdown (I don't see 
how it is useful from QMP) and they will be able to use the "shutdown" 
monitor command.

Paolo
Luiz Capitulino - April 27, 2010, 1:20 p.m.
On Tue, 27 Apr 2010 13:52:29 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 04/26/2010 09:44 PM, Luiz Capitulino wrote:
> > +    qemu_system_exit_request();
> 
> Untested suggestion: why add qemu_system_exit_request, exit_requested, 
> and a hook in the main loop?  You can do instead
> 
>     no_shutdown = 0;
>     qemu_system_shutdown_request();
> 
> which will actually call quit_timers() and net_cleanup() properly unlike 
> a blind exit(0).

 Hm, this looks good. It has the side effect of emitting the SHUTDOWN
event, but maybe this is even desirable.

 I will send a patch if there are no objections.

> Alternatively, just give an error when "quit"-ting from QMP and keep the 
> current behavior for non-QMP.  This way you do not provide two ways to 
> do the same thing.  People will have to avoid -no-shutdown (I don't see 
> how it is useful from QMP) and they will be able to use the "shutdown" 
> monitor command.

 Not sure if I got you here, why should we return an error?
Paolo Bonzini - April 27, 2010, 1:52 p.m.
On 04/27/2010 03:20 PM, Luiz Capitulino wrote:
> On Tue, 27 Apr 2010 13:52:29 +0200 Paolo Bonzini  wrote:
>> On 04/26/2010 09:44 PM, Luiz Capitulino wrote:
>>> +    qemu_system_exit_request();
>>
>> Untested suggestion: why add qemu_system_exit_request, exit_requested,
>> and a hook in the main loop?  You can do instead
>>
>>      no_shutdown = 0;
>>      qemu_system_shutdown_request();
>>
>> which will actually call quit_timers() and net_cleanup() properly unlike
>> a blind exit(0).
>
>   Hm, this looks good. It has the side effect of emitting the SHUTDOWN
> event, but maybe this is even desirable.

Exactly.

>> Alternatively, just give an error when "quit"-ting from QMP and keep the
>> current behavior for non-QMP.  This way you do not provide two ways to
>> do the same thing.  People will have to avoid -no-shutdown (I don't see
>> how it is useful from QMP) and they will be able to use the "shutdown"
>> monitor command.
>
>   Not sure if I got you here, why should we return an error?

Because quit looks like a useless duplicate of shutdown in QMP 
scenarios.  (As long as you do not pass -no-shutdown; but I don't see 
why a management app should).

Paolo

Patch

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