[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Qemu-devel] [PATCH v5 2/4] shutdown: Prepare for use of an enum in reset/shutdown_request
On 04/28/2017 09:42 AM, Markus Armbruster wrote: > Eric Blake <eblake@xxxxxxxxxx> 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 a QAPI 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 >> next patch will 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. Since QAPI >> generates enums starting at 0, it's easier if we use a different >> number as our sentinel that no request has happened yet. Most of >> the changes are in vl.c, but xen was using things externally. >> >> -static int reset_requested; >> -static int shutdown_requested, shutdown_signal; >> +static int reset_requested = -1; >> +static int shutdown_requested = -1, shutdown_signal; > > Peeking ahead, I see that shutdown_requested and reset_requested take > ShutdownCause values and -1. The latter means "no shutdown requested". > What about adding 'none' to ShutdownCause, with value 0, und use that > instead of literal -1? Would avoid the unusual "negative means false, > non-negative means true". Works nicely if the enum is internal-use only. Gets a bit more awkward if the enum is exposed to the end-user. The fact that I let QAPI generate the enum in patch 3 is evidence that I'm leaning towards exposing it to the end user (patch 4); if we want to keep it internal-only, a better place for the enum might be in sysemu.h (where we also have the weird '#define VMRESET_SILENT false' '#define VMRESET_REPORT true' to name a boolean parameter). > > PATCH 4 exposes ShutdownCause in event SHUTDOWN, and 'none' must not > occur there. However, if we ever add a query-shutdown to go with this > event, we will need 'none' there. So, query-shutdown would basically be: what is the last-reported shutdown event (normally none, when the guest is still running; but if, like libvirt, you start qemu -no-shutdown, it can then be the cause of why we are in a shutdown/stopped state while waiting for final cleanup)? How important/likely is such an event? (Hmm, from libvirt's perspective, events are usually reliable, but can be lost; if we can restart libvirtd and reconnect to a qemu process that is hanging on to life only because no one has cleaned it up yet, query-shutdown does seem like a useful thing for libvirt to have at the time it reconnects to that qemu process). We could always include 'none' in the QAPI enum, then document in 'SHUTDOWN' and 'RESET' events that the cause will never be 'none'. Doc hacks like that feel a little unclean, but not so horrible as to be unforgivable. > > I'd be tempted to reshuffle declarations here, because shutdown_signal's > int is a different one than reset_requested's and shutdown_requested, > and the latter two's "negative means false, non-negative means true" is > unusual enough to justify a comment. ... > > Hmm. In case we stick to literal -1: consider splitting this patch into > a part that changes @shutdown_requested from zero/non-zero to > negative/non-negative, and a part that uses ShutdownCause for the > non-negative values. You're definitely right that if the enum doesn't have a nice none=0 state, then reshuffling to the magic -1 as no request is awkward enough to be done alone. But part of the answer is also dependent on whether we want PATCH 4 or not (or, as you brought up, the possibility of a query-shutdown command with even more persistent storage of the last-reported event). >> @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void) >> static int qemu_reset_requested(void) >> { >> int r = reset_requested; >> - if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { >> - reset_requested = 0; >> + if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { >> + reset_requested = -1; >> return r; >> } >> - return false; >> + return -1; > > "return false" in a function returning int smells, good riddance. > In one of my earlier drafts of the patch, I even tried to change the return type from int to bool, but decided that keeping it as int worked best (if I have to use the -1/cause dichotomy). But you're also right that with a 'none' value in the enum, I could directly return ShutdownCause. >> } >> >> static int qemu_suspend_requested(void) >> @@ -1686,7 +1686,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 ShutdownType for details. Otherwise, >> + * @report is VMRESET_SILENT and @reason is ignored. >> + */ >> +void qemu_system_reset(bool report, int reason) > > Why int reason and not ShutdownCause? Hmm, peeking ahead, I see you > pass -1 with VMRESET_SILENT. Yet another place where you use int for > type ShutdownCause + { -1 }. Adding 'none' to ShutdownCause looks > even more attractive to me now. Yeah, it's getting to be that way to me to, even if it just means that I may have volunteered myself into writing a query-shutdown QMP command. >> @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation >> *info) >> void qemu_system_reset_request(void) >> { >> if (no_reboot) { >> - shutdown_requested = 1; >> + /* FIXME - add a parameter to allow callers to specify reason */ > > FIXME addressed in the next patch. Mention in this one's commit > message? Sure. Something like "Mark a couple of places as FIXME where we have to guess a value to use; a later patch will fix things to supply a correct value". > >> + shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET; I've also debated about splitting patch 3 into two parts: the event member additions (affecting .json and vl.c) and the parameter additions (affecting all other call-sites). If I add the event parameter first, then supplying a bogus value to the event means extra churn to qemu iotests output files unless I change THIS line of code to guess SHUTDOWN_CAUSE_HOST_QMP; the other option is to wire up parameter passing first and event reporting last. I'll wait for more inputs before respinning this series (I already did a poor enough job slamming mailboxes by sending 3 iterations of the series in one day). As you mention, I'd still like to hear ideas for the replay side of things, and I wouldn't mind if Dan has any ideas from the libvirt/upper-layer stack usage side of things on the fate of patch 4. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |