[libvirt] [PATCH] qemu: add two hook script events "prepare" and "release"

[Daniel Veillard]

On Mon, Mar 21, 2011 at 10:36:55AM +0100, Thibault VINCENT wrote:
> >From 827261dbe2ab8f1cfd8c84df14bb42fd04df2307 Mon Sep 17 00:00:00 2001
> From: Thibault VINCENT <thibault.vincent at smartjog.com>
> Date: Mon, 21 Mar 2011 10:18:06 +0100
> Subject: [PATCH] qemu: add two hook script events "prepare" and "release"
> 
>  * Fix for bug #618970
> 
>  * The "prepare" hook is called very early in the VM statup process
> before device labeling, so that it can allocate ressources not
> managed by libvirt, such as DRBD, or for instance create missing
> bridges and vlan interfaces.
> 
>  * To shutdown these devices, the "release" hook is also required at
> the very end of the VM destruction.
> 
> Signed-off-by: Thibault VINCENT <thibault.vincent at smartjog.com>
> ---
>  src/qemu/qemu_process.c |   26 ++++++++++++++++++++++++++
>  src/util/hooks.c        |    4 +++-
>  src/util/hooks.h        |    2 ++
>  3 files changed, 31 insertions(+), 1 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 793a43c..7831c3b 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1927,6 +1927,22 @@ int qemuProcessStart(virConnectPtr conn,
> 
>      vm->def->id = driver->nextvmid++;
> 
> +    /* Run a early hook to set-up missing devices */
> +    if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) {
> +        char *xml = virDomainDefFormat(vm->def, 0);
> +        int hookret;
> +
> +        hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
> +                    VIR_HOOK_QEMU_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN,
> NULL, xml);
> +        VIR_FREE(xml);
> +
> +        /*
> +         * If the script raised an error abort the launch
> +         */
> +        if (hookret < 0)
> +            goto cleanup;
> +    }

  Hum, the main problem I see there is that we may fail startup there
and go to cleanup. In that case the domain is not started but no hook is
called to reverse the operation. I wonder if VIR_HOOK_QEMU_OP_RELEASE
shouldn't be called in the cleanup ... if present, to avoid resource
leak when startup fails.

>      /* Must be run before security labelling */
>      VIR_DEBUG0("Preparing host devices");
>      if (qemuPrepareHostDevices(driver, vm->def) < 0)
> @@ -2419,6 +2435,16 @@ retry:
>      VIR_FREE(priv->vcpupids);
>      priv->nvcpupids = 0;
> 
> +    /* The "release" hook cleans up additional ressources */
> +    if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) {
> +        char *xml = virDomainDefFormat(vm->def, 0);
> +
> +        /* we can't stop the operation even if the script raised an
> error */
> +        virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
> +                    VIR_HOOK_QEMU_OP_RELEASE, VIR_HOOK_SUBOP_END, NULL,
> xml);
> +        VIR_FREE(xml);
> +    }
> +

  That's okay, but possibly not sufficient.

>      if (vm->newDef) {
>          virDomainDefFree(vm->def);
>          vm->def = vm->newDef;
> diff --git a/src/util/hooks.c b/src/util/hooks.c
> index 5ba2036..c258318 100644
> --- a/src/util/hooks.c
> +++ b/src/util/hooks.c
> @@ -70,8 +70,10 @@ VIR_ENUM_IMPL(virHookSubop, VIR_HOOK_SUBOP_LAST,
>                "end")
> 
>  VIR_ENUM_IMPL(virHookQemuOp, VIR_HOOK_QEMU_OP_LAST,
> +              "prepare",
>                "start",
> -              "stopped")
> +              "stopped",
> +              "release")

  prepare should be added after stopped to avoid changing the order.

>  VIR_ENUM_IMPL(virHookLxcOp, VIR_HOOK_LXC_OP_LAST,
>                "start",
> diff --git a/src/util/hooks.h b/src/util/hooks.h
> index f311ed1..66b378a 100644
> --- a/src/util/hooks.h
> +++ b/src/util/hooks.h
> @@ -52,8 +52,10 @@ enum virHookSubopType {
>  };
> 
>  enum virHookQemuOpType {
> +    VIR_HOOK_QEMU_OP_PREPARE,          /* domain startup initiated */
>      VIR_HOOK_QEMU_OP_START,            /* domain is about to start */
>      VIR_HOOK_QEMU_OP_STOPPED,          /* domain has stopped */
> +    VIR_HOOK_QEMU_OP_RELEASE,          /* domain destruction is over */

  same thing, only add at the end.

>      VIR_HOOK_QEMU_OP_LAST,
>  };

  Fixing the 2 last problems is trivial and I did it in my tree, but I
don't feel like commiting this without handling the failure to start
the domain (running VIR_HOOK_QEMU_OP_RELEASE with a new virHookSubopType
VIR_HOOK_SUBOP_FAILED for example).

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/