On Fri, Nov 14, 2008 at 05:44:29PM +0000, Daniel P. Berrange wrote:
> As per our earlier discussion today, this patch expands the callback for
> domain events so that it also gets a event type specific 'detail' field.
> This is also kept as an int, and we define enumerations for the possible
> values associated with each type.
[...]
Okay, that made the overall callbacks set clearer, ACK
> If a event type has no detail, 0 is passed.
Actually I would define a detail enum for all event type just to
make clear what the value will be and expose how it's intended to be
extended if needed.
> I don't make use of 'CRASHED' in QEMU driver yet. It might be useful in
> Xen though - when a PV guest crashes, Xen stops the domain running, but
> leaves it there in a shutoff state, but marked as crashed.
>
> Now using the C event-test program you can see the effects:
>
> myDomainEventCallback1 EVENT: Domain F9x86_64(2) Started Booted
> myDomainEventCallback2 EVENT: Domain F9x86_64(2) Started Booted
> myDomainEventCallback1 EVENT: Domain F9x86_64(-1) Stopped Destroyed
> myDomainEventCallback2 EVENT: Domain F9x86_64(-1) Stopped Destroyed
> myDomainEventCallback1 EVENT: Domain F9x86_64(3) Started Booted
> myDomainEventCallback2 EVENT: Domain F9x86_64(3) Started Booted
> myDomainEventCallback1 EVENT: Domain F9x86_64(3) Suspended
> myDomainEventCallback2 EVENT: Domain F9x86_64(3) Suspended
> myDomainEventCallback1 EVENT: Domain F9x86_64(3) Resumed
> myDomainEventCallback2 EVENT: Domain F9x86_64(3) Resumed
> myDomainEventCallback1 EVENT: Domain F9x86_64(-1) Stopped Shutdown
> myDomainEventCallback2 EVENT: Domain F9x86_64(-1) Stopped Shutdown
>
> Of the following sequence of actions
>
> virsh start F9x86_64
> virsh destroy F9x86_64
> virsh start F9x86_64
> virsh suspend F9x86_64
> virsh resume F9x86_64
> virsh shutdown F9x86_64
>
> For the last 'shutdown' operation, you'll see the same if you just run
> a graceful shutdown inside the guest itself.
okay
> NB, I've not tested saved/restored because my install of KVM is not new
> enough to support that correctly, but I expect it to work without trouble.
> Likewise for migration.
>
> A word about migration...
>
> - The destination host first gets a STARTED event, with detail MIGRATED
> when it starts running
> - The source host then gets a STOPPED event with detail MIGRATED when
> it completes
What about 'live' migration, i.e. events sent when the migration flows
begin but the domain is stil running. I don't know if KVM has this but
on Xen I would expect to be able to notice this. On the target host that
could be indicated by SUSPENDED + VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED
because it will consume the memory resource like a suspended domain but
no actual CPU cycle (well except for migration itself). On the source
host this is a bit harder to indicate, maybe this isn't needed as the
resource usage doesn't really change at that point.
> - The destination host then gets a RESUMED event, on success, and
> a STOPPED event with detail FAILED if migration aborts.
okay
> +static const char *eventDetailToString(int event, int detail) {
> + const char *ret = "";
> + switch(event) {
> + case VIR_DOMAIN_EVENT_DEFINED:
> + if (detail == VIR_DOMAIN_EVENT_DEFINED_ADDED)
> + ret = "Added";
> + else if (detail == VIR_DOMAIN_EVENT_DEFINED_UPDATED)
> + ret = "Updated";
> + break;
> + case VIR_DOMAIN_EVENT_STARTED:
> + switch (detail) {
> + case VIR_DOMAIN_EVENT_STARTED_BOOTED:
> + ret = "Booted";
> + break;
> + case VIR_DOMAIN_EVENT_STARTED_MIGRATED:
> + ret = "Migrated";
> + break;
> + case VIR_DOMAIN_EVENT_STARTED_RESTORED:
> + ret = "Restored";
> + break;
> + }
> + break;
> + case VIR_DOMAIN_EVENT_STOPPED:
> + switch (detail) {
> + case VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN:
> + ret = "Shutdown";
> + break;
> + case VIR_DOMAIN_EVENT_STOPPED_DESTROYED:
> + ret = "Destroyed";
> + break;
> + case VIR_DOMAIN_EVENT_STOPPED_CRASHED:
> + ret = "Crashed";
> + break;
> + case VIR_DOMAIN_EVENT_STOPPED_MIGRATED:
> + ret = "Migrated";
> + break;
> + case VIR_DOMAIN_EVENT_STOPPED_SAVED:
> + ret = "Failed";
> + break;
> + case VIR_DOMAIN_EVENT_STOPPED_FAILED:
> + ret = "Failed";
> + break;
> + }
> + break;
> }
> return ret;
One more reason to add enums for all cases would be to catch here
with a warning missing addition to the enums.
[...]
Patch looks fine to me, I would just add enums for all type but I
think this is still okay as-is too as this doesn't change the API/ABI
in an incompatible way.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list