[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request |
Date: |
Mon, 8 May 2017 13:33:41 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
On 05/08/2017 01:26 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 necessary here. Perhaps you
> do it for consistency with the next hunk. Do you? Just asking to make
> sure I get what you're doing.
Consistency with the next hunk, AND because a later patch then uses
'request' to pass an additional parameter to qapi_event_send_shutdown().
>
> Hmm, there's another one in xen-hvm.c, but consistency hardly applies
> there. If later patches add more uses, you might want delay the change
> until then.
Can do, if it makes incremental reviews easier.
>> +++ b/migration/savevm.c
>> @@ -2300,7 +2300,7 @@ int load_vmstate(const char *name)
>> return -EINVAL;
>> }
>>
>> - qemu_system_reset(VMRESET_SILENT);
>> + qemu_system_reset(VMRESET_SILENT, SHUTDOWN_CAUSE_NONE);
>> mis->from_src_file = f;
>>
>> aio_context_acquire(aio_context);
>
> You seem to be passing SHUTDOWN_CAUSE_NONE exactly with VMRESET_SILENT.
> Would it be possible to have SHUTDOWN_CAUSE_NONE imply !report, any
> other case imply report, and get rid of the first parameter?
Indeed, and it would also get rid of the ugly
#define VMRESET_SILENT false
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature