[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request |
Date: |
Tue, 09 May 2017 13:30:01 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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...
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,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.
Use your judgement.
>>> +++ 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
I'd love that.