Re: [Xen-devel] [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request

[Markus Armbruster]

Eric Blake  writes:

> On 05/08/2017 01:26 PM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> We want to track why a guest was shutdown; in particular, being able
>>> to tell the difference between a guest request (such as ACPI request)
>>> and host request (such as SIGINT) will prove useful to libvirt.
>>> Since all requests eventually end up changing shutdown_requested in
>>> vl.c, the logical change is to make that value track the reason,
>>> rather than its current 0/1 contents.
>>>
>>> Since command-line options control whether a reset request is turned
>>> into a shutdown request instead, the same treatment is given to
>>> reset_requested.
>>>
>>> This patch adds an internal enum ShutdownCause that describes reasons
>>> that a shutdown can be requested, and changes qemu_system_reset() to
>>> pass the reason through, although for now it is not reported.  The
>>> enum could be exported via QAPI at a later date, if deemed necessary,
>>> but for now, there has not been a request to expose that much detail
>>> to end clients.
>>>
>>> For now, we only populate the reason with HOST_ERROR, along with FIXME
>>> comments that describe our plans for how to pass an actual correct
>>> reason.
>> 
>> In other words, replacing 0 by SHUTDOWN_CAUSE_NONE, and 1 by
>> SHUTDOWN_CAUSE_HOST_ERROR.  Makes sense.
>
> Maybe I could have ordered HOST_ERROR to actually be 1...

Might be marginally worthwhile if you can split patches so that the one
replacing int by ShutdownCause doesn't change anything but names.

>>> +/* Enumeration of various causes for shutdown. */
>>> +typedef enum ShutdownCause ShutdownCause;
>>> +enum ShutdownCause {
>> 
>> Why define the typedef separately here?  What's wrong with
>> 
>> typedef enum ShutdownCause {
>> ...
>> } ShutdownCause;
>> 
>> ?
>
> That would work too.  I don't know if the code base has a strong
> preference for one form over the other.

I don't have numbers, but I think we use the split form pretty much only
when there's a reason for the split, such as defining an incomplete type
in a header, and completing it elsewhere.

>>> +SHUTDOWN_CAUSE_NONE,  /* No shutdown requested yet */
>> 
>> Comment is fine.  Possible alternative: /* No shutdown request pending */
>> 
>>> +SHUTDOWN_CAUSE_HOST_QMP,  /* Reaction to a QMP command, like 
>>> 'quit' */
>>> +SHUTDOWN_CAUSE_HOST_SIGNAL,   /* Reaction to a signal, such as SIGINT 
>>> */
>>> +SHUTDOWN_CAUSE_HOST_UI,   /* Reaction to UI event, like window 
>>> close */
>>> +SHUTDOWN_CAUSE_HOST_ERROR,/* An error prevents further use of 
>>> guest */
>
> ...rather than 4.  I don't know that it matters much.
>
>
>>> -static int qemu_reset_requested(void)
>>> +static ShutdownCause qemu_reset_requested(void)
>>>  {
>>> -int r = reset_requested;
>>> +ShutdownCause r = reset_requested;
>> 
>> Good opportunity to insert a blank line here.
>> 
>
> Sure.
>
>>>  if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
>>> -reset_requested = 0;
>>> +reset_requested = SHUTDOWN_CAUSE_NONE;
>>>  return r;
>>>  }
>>> -return false;
>>> +return SHUTDOWN_CAUSE_NONE;
>>>  }
>>>
>>>  static int qemu_suspend_requested(void)
>>> @@ -1686,7 +1687,12 @@ static int qemu_debug_requested(void)
>>>  return r;
>>>  }
>>>
>>> -void qemu_system_reset(bool report)
>>> +/*
>>> + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using
>>> + * the @reason interpreted as ShutdownCause for details.  Otherwise,
>>> + * @report is VMRESET_SILENT and @reason is ignored.
>>> + */
>> 
>> "interpreted as ShutdownCause"?  It *is* a ShutdownCause.  Leftover?
>
> Oh, yeah. In v5, the parameter was 'int'.

Easy enough to clean up :)

>>> +void qemu_system_reset(bool report, ShutdownCause reason)
>>>  {
>>>  MachineClass *mc;
>>>
>>> @@ -1700,6 +1706,7 @@ void qemu_system_reset(bool report)
>>>  qemu_devices_reset();
>>>  }
>>>  if (report) {
>>> +assert(reason);
>>>  qapi_event_send_reset(_abort);
>>>  }
>>>  cpu_synchronize_all_post_reset();
>> 
>> Looks like we're not using @reason "for details" just yet.
>
> Correct. I can add a FIXME (to be removed in the later patch where it is
> used) if that is desired.

Not necessary if the function comment refrains from claiming it *is*
used.

>>> @@ -1807,7 +1815,7 @@ void qemu_system_killed(int signal, pid_t pid)
>>>  /* Cannot call qemu_system_shutdown_request directly because
>>>   * we are in a signal handler.
>>>   */
>>> -shutdown_requested = 1;
>>> +shutdown_requested = SHUTDOWN_CAUSE_HOST_SIGNAL;
>> 
>> Should this be SHUTDOWN_CAUSE_HOST_ERROR, to be updated in the next
>> patch?  Alternatively, tweak this patch's commit message?
>
> This is the one case that we actually do have a strong cause affiliated
> with the reason without having to resort to changing function
> signatures.  Commit message tweak is 

Re: [Xen-devel] [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request

[Eric Blake]

On 05/08/2017 01:26 PM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> We want to track why a guest was shutdown; in particular, being able
>> to tell the difference between a guest request (such as ACPI request)
>> and host request (such as SIGINT) will prove useful to libvirt.
>> Since all requests eventually end up changing shutdown_requested in
>> vl.c, the logical change is to make that value track the reason,
>> rather than its current 0/1 contents.
>>
>> Since command-line options control whether a reset request is turned
>> into a shutdown request instead, the same treatment is given to
>> reset_requested.
>>
>> This patch adds an internal enum ShutdownCause that describes reasons
>> that a shutdown can be requested, and changes qemu_system_reset() to
>> pass the reason through, although for now it is not reported.  The
>> enum could be exported via QAPI at a later date, if deemed necessary,
>> but for now, there has not been a request to expose that much detail
>> to end clients.
>>
>> For now, we only populate the reason with HOST_ERROR, along with FIXME
>> comments that describe our plans for how to pass an actual correct
>> reason.
> 
> In other words, replacing 0 by SHUTDOWN_CAUSE_NONE, and 1 by
> SHUTDOWN_CAUSE_HOST_ERROR.  Makes sense.

Maybe I could have ordered HOST_ERROR to actually be 1...


>> +/* Enumeration of various causes for shutdown. */
>> +typedef enum ShutdownCause ShutdownCause;
>> +enum ShutdownCause {
> 
> Why define the typedef separately here?  What's wrong with
> 
> typedef enum ShutdownCause {
> ...
> } ShutdownCause;
> 
> ?

That would work too.  I don't know if the code base has a strong
preference for one form over the other.

> 
>> +SHUTDOWN_CAUSE_NONE,  /* No shutdown requested yet */
> 
> Comment is fine.  Possible alternative: /* No shutdown request pending */
> 
>> +SHUTDOWN_CAUSE_HOST_QMP,  /* Reaction to a QMP command, like 'quit' 
>> */
>> +SHUTDOWN_CAUSE_HOST_SIGNAL,   /* Reaction to a signal, such as SIGINT */
>> +SHUTDOWN_CAUSE_HOST_UI,   /* Reaction to UI event, like window 
>> close */
>> +SHUTDOWN_CAUSE_HOST_ERROR,/* An error prevents further use of guest 
>> */

...rather than 4.  I don't know that it matters much.


>> -static int qemu_reset_requested(void)
>> +static ShutdownCause qemu_reset_requested(void)
>>  {
>> -int r = reset_requested;
>> +ShutdownCause r = reset_requested;
> 
> Good opportunity to insert a blank line here.
> 

Sure.

>>  if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
>> -reset_requested = 0;
>> +reset_requested = SHUTDOWN_CAUSE_NONE;
>>  return r;
>>  }
>> -return false;
>> +return SHUTDOWN_CAUSE_NONE;
>>  }
>>
>>  static int qemu_suspend_requested(void)
>> @@ -1686,7 +1687,12 @@ static int qemu_debug_requested(void)
>>  return r;
>>  }
>>
>> -void qemu_system_reset(bool report)
>> +/*
>> + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using
>> + * the @reason interpreted as ShutdownCause for details.  Otherwise,
>> + * @report is VMRESET_SILENT and @reason is ignored.
>> + */
> 
> "interpreted as ShutdownCause"?  It *is* a ShutdownCause.  Leftover?

Oh, yeah. In v5, the parameter was 'int'.

> 
>> +void qemu_system_reset(bool report, ShutdownCause reason)
>>  {
>>  MachineClass *mc;
>>
>> @@ -1700,6 +1706,7 @@ void qemu_system_reset(bool report)
>>  qemu_devices_reset();
>>  }
>>  if (report) {
>> +assert(reason);
>>  qapi_event_send_reset(_abort);
>>  }
>>  cpu_synchronize_all_post_reset();
> 
> Looks like we're not using @reason "for details" just yet.

Correct. I can add a FIXME (to be removed in the later patch where it is
used) if that is desired.


>> @@ -1807,7 +1815,7 @@ void qemu_system_killed(int signal, pid_t pid)
>>  /* Cannot call qemu_system_shutdown_request directly because
>>   * we are in a signal handler.
>>   */
>> -shutdown_requested = 1;
>> +shutdown_requested = SHUTDOWN_CAUSE_HOST_SIGNAL;
> 
> Should this be SHUTDOWN_CAUSE_HOST_ERROR, to be updated in the next
> patch?  Alternatively, tweak this patch's commit message?

This is the one case that we actually do have a strong cause affiliated
with the reason without having to resort to changing function
signatures.  Commit message tweak is better.

>> @@ -1846,13 +1855,16 @@ void qemu_system_debug_request(void)
>>  static bool main_loop_should_exit(void)
>>  {
>>  RunState r;
>> +ShutdownCause request;
>> +
>>  if (qemu_debug_requested()) {
>>  vm_stop(RUN_STATE_DEBUG);
>>  }
>>  if (qemu_suspend_requested()) {
>>  qemu_system_suspend();
>>  }
>> -if (qemu_shutdown_requested()) {
>> +request = qemu_shutdown_requested();
>> +if (request) {
>>  qemu_kill_report();
>>  qapi_event_send_shutdown(_abort);
>>  if (no_shutdown) {
> 
> The detour through @request appears 

Re: [Xen-devel] [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request

[Markus Armbruster]

Eric Blake  writes:

> We want to track why a guest was shutdown; in particular, being able
> to tell the difference between a guest request (such as ACPI request)
> and host request (such as SIGINT) will prove useful to libvirt.
> Since all requests eventually end up changing shutdown_requested in
> vl.c, the logical change is to make that value track the reason,
> rather than its current 0/1 contents.
>
> Since command-line options control whether a reset request is turned
> into a shutdown request instead, the same treatment is given to
> reset_requested.
>
> This patch adds an internal enum ShutdownCause that describes reasons
> that a shutdown can be requested, and changes qemu_system_reset() to
> pass the reason through, although for now it is not reported.  The
> enum could be exported via QAPI at a later date, if deemed necessary,
> but for now, there has not been a request to expose that much detail
> to end clients.
>
> For now, we only populate the reason with HOST_ERROR, along with FIXME
> comments that describe our plans for how to pass an actual correct
> reason.

In other words, replacing 0 by SHUTDOWN_CAUSE_NONE, and 1 by
SHUTDOWN_CAUSE_HOST_ERROR.  Makes sense.

>  The next patches will then actually wire things up to modify
> events to report data based on the reason, and to pass the correct enum
> value in from various call-sites that can trigger a reset/shutdown (big
> enough that it was worth splitting from this patch).
>
> Signed-off-by: Eric Blake 
>
> ---
> v6: make ShutdownCause internal-only, add SHUTDOWN_CAUSE_NONE so that
> comparison to 0 still works, tweak initial FIXME values
> v5: no change
> v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution
> v3: new patch
> ---
>  include/sysemu/sysemu.h | 22 ++---
>  vl.c| 51 
> +++--
>  hw/i386/xen/xen-hvm.c   |  7 +--
>  migration/colo.c|  2 +-
>  migration/savevm.c  |  2 +-
>  5 files changed, 58 insertions(+), 26 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 16175f7..e4da9d4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -36,6 +36,22 @@ void vm_state_notify(int running, RunState state);
>  #define VMRESET_SILENT   false
>  #define VMRESET_REPORT   true
>
> +/* Enumeration of various causes for shutdown. */
> +typedef enum ShutdownCause ShutdownCause;
> +enum ShutdownCause {

Why define the typedef separately here?  What's wrong with

typedef enum ShutdownCause {
...
} ShutdownCause;

?

> +SHUTDOWN_CAUSE_NONE,  /* No shutdown requested yet */

Comment is fine.  Possible alternative: /* No shutdown request pending */

> +SHUTDOWN_CAUSE_HOST_QMP,  /* Reaction to a QMP command, like 'quit' 
> */
> +SHUTDOWN_CAUSE_HOST_SIGNAL,   /* Reaction to a signal, such as SIGINT */
> +SHUTDOWN_CAUSE_HOST_UI,   /* Reaction to UI event, like window close 
> */
> +SHUTDOWN_CAUSE_HOST_ERROR,/* An error prevents further use of guest 
> */
> +SHUTDOWN_CAUSE_GUEST_SHUTDOWN,/* Guest requested shutdown, such as via
> + ACPI or other hardware-specific means */
> +SHUTDOWN_CAUSE_GUEST_RESET,   /* Guest requested reset, and command line
> + turns that into a shutdown */
> +SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
> + that into a shutdown */
> +};
> +
>  void vm_start(void);
>  int vm_prepare_start(void);
>  int vm_stop(RunState state);
> @@ -62,10 +78,10 @@ void qemu_system_debug_request(void);
>  void qemu_system_vmstop_request(RunState reason);
>  void qemu_system_vmstop_request_prepare(void);
>  bool qemu_vmstop_requested(RunState *r);
> -int qemu_shutdown_requested_get(void);
> -int qemu_reset_requested_get(void);
> +ShutdownCause qemu_shutdown_requested_get(void);
> +ShutdownCause qemu_reset_requested_get(void);
>  void qemu_system_killed(int signal, pid_t pid);
> -void qemu_system_reset(bool report);
> +void qemu_system_reset(bool report, ShutdownCause reason);
>  void qemu_system_guest_panicked(GuestPanicInformation *info);
>  size_t qemu_target_page_size(void);
>
> diff --git a/vl.c b/vl.c
> index f22a3ac..6069fb2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1597,8 +1597,9 @@ void vm_state_notify(int running, RunState state)
>  }
>  }
>
> -static int reset_requested;
> -static int shutdown_requested, shutdown_signal;
> +static ShutdownCause reset_requested;
> +static ShutdownCause shutdown_requested;
> +static int shutdown_signal;
>  static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
> @@ -1612,19 +1613,19 @@ static NotifierList wakeup_notifiers =
>  NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
>  static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
>
> -int