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

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(a)smartjog.com&gt; > 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(a)smartjog.com&gt; > --- > 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).