[Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Dominik Csapak]

when '-no-reboot' is set, it is interesting if the guest was originally
shutdown or reset, so save and return that info

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qapi/run-state.json | 5 ++++-
 vl.c                | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 332e44897b..ec1769777d 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -107,6 +107,9 @@
 # a guest-initiated ACPI shutdown request or other hardware-specific action)
 # rather than a host request (such as sending qemu a SIGINT). (since 2.10)
 #
+# @was_reset: If true, the shutdown was actually a reset, but no-reboot
+# was set, so it got converted to a shutdown
+#
 # Note: If the command-line option "-no-shutdown" has been specified, qemu will
 # not exit, and a STOP event will eventually follow the SHUTDOWN event
 #
@@ -118,7 +121,7 @@
 #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
 #
 ##
-{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool' } }
+{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool', 'was_reset': 'bool' } }
 
 ##
 # @POWERDOWN:
diff --git a/vl.c b/vl.c
index 4e25c78bff..66e29cb830 100644
--- a/vl.c
+++ b/vl.c
@@ -1524,6 +1524,7 @@ void vm_state_notify(int running, RunState state)
 
 static ShutdownCause reset_requested;
 static ShutdownCause shutdown_requested;
+static bool shutdown_was_reset;
 static int shutdown_signal;
 static pid_t shutdown_pid;
 static int powerdown_requested;
@@ -1681,6 +1682,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
 void qemu_system_reset_request(ShutdownCause reason)
 {
     if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
+        shutdown_was_reset = true;
         shutdown_requested = reason;
     } else {
         reset_requested = reason;
@@ -1807,7 +1809,8 @@ static bool main_loop_should_exit(void)
     request = qemu_shutdown_requested();
     if (request) {
         qemu_kill_report();
-        qapi_event_send_shutdown(shutdown_caused_by_guest(request));
+        qapi_event_send_shutdown(shutdown_caused_by_guest(request),
+                                 shutdown_was_reset);
         if (no_shutdown) {
             vm_stop(RUN_STATE_SHUTDOWN);
         } else {
-- 
2.11.0

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Dominik Csapak]

On 10/8/18 3:19 PM, Dominik Csapak wrote:
> when '-no-reboot' is set, it is interesting if the guest was originally
> shutdown or reset, so save and return that info
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   qapi/run-state.json | 5 ++++-
>   vl.c                | 5 ++++-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 332e44897b..ec1769777d 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -107,6 +107,9 @@
>   # a guest-initiated ACPI shutdown request or other hardware-specific action)
>   # rather than a host request (such as sending qemu a SIGINT). (since 2.10)
>   #
> +# @was_reset: If true, the shutdown was actually a reset, but no-reboot
> +# was set, so it got converted to a shutdown
> +#
>   # Note: If the command-line option "-no-shutdown" has been specified, qemu will
>   # not exit, and a STOP event will eventually follow the SHUTDOWN event
>   #
> @@ -118,7 +121,7 @@
>   #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
>   #
>   ##
> -{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool' } }
> +{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool', 'was_reset': 'bool' } }
>   
>   ##
>   # @POWERDOWN:
> diff --git a/vl.c b/vl.c
> index 4e25c78bff..66e29cb830 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1524,6 +1524,7 @@ void vm_state_notify(int running, RunState state)
>   
>   static ShutdownCause reset_requested;
>   static ShutdownCause shutdown_requested;
> +static bool shutdown_was_reset;
>   static int shutdown_signal;
>   static pid_t shutdown_pid;
>   static int powerdown_requested;
> @@ -1681,6 +1682,7 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>   void qemu_system_reset_request(ShutdownCause reason)
>   {
>       if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
> +        shutdown_was_reset = true;
>           shutdown_requested = reason;
>       } else {
>           reset_requested = reason;
> @@ -1807,7 +1809,8 @@ static bool main_loop_should_exit(void)
>       request = qemu_shutdown_requested();
>       if (request) {
>           qemu_kill_report();
> -        qapi_event_send_shutdown(shutdown_caused_by_guest(request));
> +        qapi_event_send_shutdown(shutdown_caused_by_guest(request),
> +                                 shutdown_was_reset);
>           if (no_shutdown) {
>               vm_stop(RUN_STATE_SHUTDOWN);
>           } else {
> 

hi,

any comment on this?

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Paolo Bonzini]

On 17/10/2018 09:17, Dominik Csapak wrote:
> On 10/8/18 3:19 PM, Dominik Csapak wrote:
>> when '-no-reboot' is set, it is interesting if the guest was originally
>> shutdown or reset, so save and return that info 
>
> 
>   {
>       if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
> +        shutdown_was_reset = true;
>           shutdown_requested = reason;
>       } else {
>           reset_requested = reason;
> @@ -1807,7 +1809,8 @@ static bool main_loop_should_exit(void)
>       request = qemu_shutdown_requested();
>       if (request) {
>           qemu_kill_report();
> -        qapi_event_send_shutdown(shutdown_caused_by_guest(request));
> +        qapi_event_send_shutdown(shutdown_caused_by_guest(request),
> +                                 shutdown_was_reset);

So the problem with shutdown_caused_by_guest is that you get the same
value for both guest reset and guest shutdown.  Could we instead just
pass the ShutdownCause in the event (similar to what was proposed even
when discussing commit 08fba7ac9b618516a5f1d096f78a7e2837fe0594)?

You would get either guest-reset or host-qmp if it's a reset; the same
host-qmp reason could account for both "quit" and "system_reset", but in
practice the caller will know if it has asked for a hard shutdown or a
reset.

Paolo

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Dominik Csapak]

On 10/17/18 12:58 PM, Paolo Bonzini wrote:
> On 17/10/2018 09:17, Dominik Csapak wrote:
>> On 10/8/18 3:19 PM, Dominik Csapak wrote:
>>> when '-no-reboot' is set, it is interesting if the guest was originally
>>> shutdown or reset, so save and return that info
>>
>>
>>    {
>>        if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
>> +        shutdown_was_reset = true;
>>            shutdown_requested = reason;
>>        } else {
>>            reset_requested = reason;
>> @@ -1807,7 +1809,8 @@ static bool main_loop_should_exit(void)
>>        request = qemu_shutdown_requested();
>>        if (request) {
>>            qemu_kill_report();
>> -        qapi_event_send_shutdown(shutdown_caused_by_guest(request));
>> +        qapi_event_send_shutdown(shutdown_caused_by_guest(request),
>> +                                 shutdown_was_reset);
> 
> So the problem with shutdown_caused_by_guest is that you get the same
> value for both guest reset and guest shutdown.  Could we instead just
> pass the ShutdownCause in the event (similar to what was proposed even
> when discussing commit 08fba7ac9b618516a5f1d096f78a7e2837fe0594)?
> 
> You would get either guest-reset or host-qmp if it's a reset; the same
> host-qmp reason could account for both "quit" and "system_reset", but in
> practice the caller will know if it has asked for a hard shutdown or a
> reset.

i would find it nicer to always be able to distinguish between
a reset and a normal exit, since it would be less work on the
management side (i.e. we would have to handle the qmp reset elsewhere
instead of in the same process that monitors the events)

also it would make the 'guest' parameter redundant or change the api and 
i am not sure about how stable it has to be?

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Paolo Bonzini]

On 17/10/2018 13:34, Dominik Csapak wrote:
> i would find it nicer to always be able to distinguish between
> a reset and a normal exit, since it would be less work on the
> management side (i.e. we would have to handle the qmp reset elsewhere
> instead of in the same process that monitors the events)

Ok---I am not opposed to this patch at all.  I mentioned ShutdownCause
only because it was brought up as a possible future extension when
'guest' was added.

> also it would make the 'guest' parameter redundant or change the api and
> i am not sure about how stable it has to be?

The guest parameter would have to stay even if it is redundant.

Paolo

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Eric Blake]

On 10/17/18 5:58 AM, Paolo Bonzini wrote:

>> -        qapi_event_send_shutdown(shutdown_caused_by_guest(request));
>> +        qapi_event_send_shutdown(shutdown_caused_by_guest(request),
>> +                                 shutdown_was_reset);
> 
> So the problem with shutdown_caused_by_guest is that you get the same
> value for both guest reset and guest shutdown.  Could we instead just
> pass the ShutdownCause in the event (similar to what was proposed even
> when discussing commit 08fba7ac9b618516a5f1d096f78a7e2837fe0594)?

Indeed, it sounds like we are now at the point where we want to do 
precisely that - expose more fine-grained details by adding 
ShutdownCause as a QAPI enum, rather than just adding another bool per 
reason.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Paolo Bonzini]

On 17/10/2018 15:54, Eric Blake wrote:
>>
>> So the problem with shutdown_caused_by_guest is that you get the same
>> value for both guest reset and guest shutdown.  Could we instead just
>> pass the ShutdownCause in the event (similar to what was proposed even
>> when discussing commit 08fba7ac9b618516a5f1d096f78a7e2837fe0594)?
> 
> Indeed, it sounds like we are now at the point where we want to do
> precisely that - expose more fine-grained details by adding
> ShutdownCause as a QAPI enum, rather than just adding another bool per
> reason.

So should we split HOST_QMP into HOST_QMP_{SYSTEM_RESET,QUIT} and add
the ShutdownCause instead of was-reset?

Paolo

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Dominik Csapak]

On 10/17/18 4:43 PM, Paolo Bonzini wrote:
> On 17/10/2018 15:54, Eric Blake wrote:
>>>
>>> So the problem with shutdown_caused_by_guest is that you get the same
>>> value for both guest reset and guest shutdown.  Could we instead just
>>> pass the ShutdownCause in the event (similar to what was proposed even
>>> when discussing commit 08fba7ac9b618516a5f1d096f78a7e2837fe0594)?
>>
>> Indeed, it sounds like we are now at the point where we want to do
>> precisely that - expose more fine-grained details by adding
>> ShutdownCause as a QAPI enum, rather than just adding another bool per
>> reason.
> 
> So should we split HOST_QMP into HOST_QMP_{SYSTEM_RESET,QUIT} and add
> the ShutdownCause instead of was-reset?
> 
> Paolo
> 
> 

this would work for us, should i send patches for this?
i would (roughly) do this:

move shutdowncause to qapi json
split host_qmp into system_reset and quit
and add the shutdowncause to the shutdown event
fix iotests

with kind regards
Dominik

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Eric Blake]

On 10/17/18 2:17 AM, Dominik Csapak wrote:
> On 10/8/18 3:19 PM, Dominik Csapak wrote:
>> when '-no-reboot' is set, it is interesting if the guest was originally
>> shutdown or reset, so save and return that info
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   qapi/run-state.json | 5 ++++-
>>   vl.c                | 5 ++++-
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>> index 332e44897b..ec1769777d 100644
>> --- a/qapi/run-state.json
>> +++ b/qapi/run-state.json
>> @@ -107,6 +107,9 @@
>>   # a guest-initiated ACPI shutdown request or other hardware-specific 
>> action)
>>   # rather than a host request (such as sending qemu a SIGINT). (since 
>> 2.10)
>>   #
>> +# @was_reset: If true, the shutdown was actually a reset, but no-reboot
>> +# was set, so it got converted to a shutdown

New additions should prefer naming like 'was-reset' rather than 
'was_reset', if we still think this particular name is appropriate.  My 
personal take: what does the 'was' add, which would prevent us from just 
using the name 'reset' and avoiding the separator spelling issue?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] qapi: add info about reset to SHUTDOWN event

[Dominik Csapak]

On 10/17/18 3:51 PM, Eric Blake wrote:
> On 10/17/18 2:17 AM, Dominik Csapak wrote:
>> On 10/8/18 3:19 PM, Dominik Csapak wrote:
>>> when '-no-reboot' is set, it is interesting if the guest was originally
>>> shutdown or reset, so save and return that info
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   qapi/run-state.json | 5 ++++-
>>>   vl.c                | 5 ++++-
>>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>>> index 332e44897b..ec1769777d 100644
>>> --- a/qapi/run-state.json
>>> +++ b/qapi/run-state.json
>>> @@ -107,6 +107,9 @@
>>>   # a guest-initiated ACPI shutdown request or other 
>>> hardware-specific action)
>>>   # rather than a host request (such as sending qemu a SIGINT). 
>>> (since 2.10)
>>>   #
>>> +# @was_reset: If true, the shutdown was actually a reset, but no-reboot
>>> +# was set, so it got converted to a shutdown
> 
> New additions should prefer naming like 'was-reset' rather than 
> 'was_reset', if we still think this particular name is appropriate.  My 
> personal take: what does the 'was' add, which would prevent us from just 
> using the name 'reset' and avoiding the separator spelling issue?
> 

yes of course 'reset' is fine, i just thought it might be confusing
having a RESET event and a SHUTDOWN event with a 'reset' flag, but
i guess if one tries to monitor them they would read the
api documentation anyway