[libvirt] PATCH: Add domain event "detail" information

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/