[libvirt] [PATCH 3/4] libxl: handle domain shutdown events in a thread

[Michal Privoznik]

On 05.02.2014 18:39, Jim Fehlig wrote:
> Handling the domain shutdown event within the event handler seems
> a bit unfair to libxl's event machinery.  Domain "shutdown" could
> take considerable time.  E.g. if the shutdown reason is reboot,
> the domain must be reaped and then started again.
>
> Spawn a shutdown handler thread to do this work, allowing libxl's
> event machinery to go about its business.
>
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>   src/libxl/libxl_driver.c | 132 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 89 insertions(+), 43 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d639011..a1c6c0f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -352,61 +352,107 @@ libxlVmReap(libxlDriverPrivatePtr driver,
>   # define VIR_LIBXL_EVENT_CONST const
>   #endif
>
> +struct libxlShutdownThreadInfo
> +{
> +    virDomainObjPtr vm;
> +    libxl_event *event;
> +};
> +
> +
>   static void
> -libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
> +libxlDomainShutdownThread(void *opaque)
>   {
>       libxlDriverPrivatePtr driver = libxl_driver;
> -    libxlDomainObjPrivatePtr priv = ((virDomainObjPtr)data)->privateData;
> -    virDomainObjPtr vm = NULL;
> +    struct libxlShutdownThreadInfo *shutdown_info = opaque;
> +    virDomainObjPtr vm = shutdown_info->vm;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
> +    libxl_event *ev = shutdown_info->event;
> +    libxl_ctx *ctx = priv->ctx;
>       virObjectEventPtr dom_event = NULL;
> -    libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
> -
> -    if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> -        virDomainShutoffReason reason;
> -
> -        /*
> -         * Similar to the xl implementation, ignore SUSPEND.  Any actions needed
> -         * after calling libxl_domain_suspend() are handled by it's callers.
> -         */
> -        if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> -            goto cleanup;
> +    libxl_shutdown_reason xl_reason = ev->u.domain_shutdown.shutdown_reason;
> +    virDomainShutoffReason reason;
>
> -        vm = virDomainObjListFindByID(driver->domains, event->domid);
> -        if (!vm)
> -            goto cleanup;
> +    virObjectLock(vm);
>
> -        switch (xl_reason) {
> -            case LIBXL_SHUTDOWN_REASON_POWEROFF:
> -            case LIBXL_SHUTDOWN_REASON_CRASH:
> -                if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
> -                    dom_event = virDomainEventLifecycleNewFromObj(vm,
> -                                              VIR_DOMAIN_EVENT_STOPPED,
> -                                              VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> -                    reason = VIR_DOMAIN_SHUTOFF_CRASHED;
> -                } else {
> -                    reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
> -                }
> -                libxlVmReap(driver, vm, reason);
> -                if (!vm->persistent) {
> -                    virDomainObjListRemove(driver->domains, vm);
> -                    vm = NULL;
> -                }
> -                break;
> -            case LIBXL_SHUTDOWN_REASON_REBOOT:
> -                libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> -                libxlVmStart(driver, vm, 0, -1);
> -                break;
> -            default:
> -                VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> -                break;
> -        }
> +    switch (xl_reason) {
> +        case LIBXL_SHUTDOWN_REASON_POWEROFF:
> +        case LIBXL_SHUTDOWN_REASON_CRASH:
> +            if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
> +                dom_event = virDomainEventLifecycleNewFromObj(vm,
> +                                           VIR_DOMAIN_EVENT_STOPPED,
> +                                           VIR_DOMAIN_EVENT_STOPPED_CRASHED);
> +                reason = VIR_DOMAIN_SHUTOFF_CRASHED;
> +            } else {
> +                reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
> +            }
> +            libxlVmReap(driver, vm, reason);
> +            if (!vm->persistent) {
> +                virDomainObjListRemove(driver->domains, vm);
> +                vm = NULL;
> +            }
> +            break;
> +        case LIBXL_SHUTDOWN_REASON_REBOOT:
> +            libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> +            libxlVmStart(driver, vm, 0, -1);
> +            break;
> +        default:
> +            VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> +            break;
>       }
>
> -cleanup:
>       if (vm)
>           virObjectUnlock(vm);
>       if (dom_event)
>           libxlDomainEventQueue(driver, dom_event);
> +    libxl_event_free(ctx, ev);
> +    VIR_FREE(shutdown_info);
> +}
> +
> +static void
> +libxlEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event *event)
> +{
> +    virDomainObjPtr vm = data;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
> +    libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason;
> +    struct libxlShutdownThreadInfo *shutdown_info;
> +    virThread thread;
> +
> +    if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> +        VIR_INFO("Unhandled event type %d", event->type);
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * Similar to the xl implementation, ignore SUSPEND.  Any actions needed
> +     * after calling libxl_domain_suspend() are handled by it's callers.
> +     */
> +    if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND)
> +        goto cleanup;
> +
> +    /*
> +     * Start a thread to handle shutdown.  We don't want to be tying up
> +     * libxl's event machinery by doing a potentially lengthy shutdown.
> +     */
> +    if (VIR_ALLOC(shutdown_info) < 0)
> +        goto cleanup;
> +
> +    shutdown_info->vm = data;
> +    shutdown_info->event = (libxl_event *)event;
> +    if (virThreadCreate(&thread, true, libxlDomainShutdownThread,
> +                        shutdown_info) < 0) {

The 2nd argument 'true' tells, if @thread is joinable. Since you are not 
joining it anywhere, I guess it should be rather 'false'. Moreover, it 
will avoid some memory leaks, as pthread free()-s memory needed for 
thread itself (otherwise the memory would be free()-d in phtread_join - 
which again is not called anywhere).

> +        /*
> +         * Not much we can do on error here except log it.
> +         */
> +        VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * libxl_event freed in shutdown thread
> +     */
> +    return;
> +
> +cleanup:

Since this is in fact an error path, I suggest renaming the label to 
'error'.

>       /* Cast away any const */
>       libxl_event_free(priv->ctx, (libxl_event *)event);
>   }
>

ACK if you fix these two issues.

Michal