Re: [Qemu-devel] [PATCH 5/9] Monitor: Return before exiting with 'quit'

[Anthony Liguori <anthony@codemonkey.ws>]

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();
>   }
>