Re: [Xen-devel] [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request
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 qemu_shutdown_requested_get(void) > +Shu
Re: [Xen-devel] [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request
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(&error_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(&error_abort); >> if (no_shutdown) { > > The detour through @request appears isn't n
Re: [Xen-devel] [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request
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(&error_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 better. Works for me. >>> @@ -1846,