[libvirt] [PATCH 0/3] libxl hooks
Hi there! Here is small patchset adding hooks support to the libxl driver. Cédric Bosdonnat (3): libxl: add a flag to mark guests as tainted by a hook libxl: fix segfault in libxlReconnectDomain libxl: add hooks support docs/hooks.html.in | 53 ++++++++++++++++++++++++++-- src/libxl/libxl_domain.c | 85 +++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_domain.h | 2 ++ src/libxl/libxl_driver.c | 52 ++++++++++++++++++++------- src/libxl/libxl_migration.c | 58 +++++++++++++++++++++++++++++++ src/util/virhook.c | 16 ++++++++- src/util/virhook.h | 13 +++++++ 7 files changed, 264 insertions(+), 15 deletions(-) -- 2.6.6
The migrate hook will affect the migrated guest definition. Allow these domains be marked as tainted in the libxl driver. --- src/libxl/libxl_domain.c | 10 ++++++++++ src/libxl/libxl_domain.h | 2 ++ 2 files changed, 12 insertions(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0e26b91..886b40f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1135,6 +1135,16 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def, hostdev_flags) < 0) goto cleanup_dom; + if (priv->hookRun) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + + VIR_WARN("Domain id='%d' name='%s' uuid='%s' is tainted: hook", + vm->def->id, + vm->def->name, + uuidstr); + } + /* Unlock virDomainObj while creating the domain */ virObjectUnlock(vm); diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index af11a2c..3a3890b 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -69,6 +69,8 @@ struct _libxlDomainObjPrivate { char *lockState; struct libxlDomainJobObj job; + + bool hookRun; /* true if there was a hook run over this domain */ }; -- 2.6.6
In case of error, libxlReconnectDomain may call virDomainObjListRemoveLocked. However it has no local reference on the domain object, leading to segfault. Get a reference to the domain object at the start of the function and release it at the end to avoid problems. This commit also factorizes code between the error and normal ends. --- src/libxl/libxl_driver.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index cb501cf..5008c00 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -373,11 +373,13 @@ libxlReconnectDomain(virDomainObjPtr vm, uint8_t *data = NULL; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; + int ret = -1; #ifdef LIBXL_HAVE_PVUSB hostdev_flags |= VIR_HOSTDEV_SP_USB; #endif + virObjectRef(vm); virObjectLock(vm); libxl_dominfo_init(&d_info); @@ -385,18 +387,18 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Does domain still exist? */ rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id); if (rc == ERROR_INVAL) { - goto out; + goto error; } else if (rc != 0) { VIR_DEBUG("libxl_domain_info failed (code %d), ignoring domain %d", rc, vm->def->id); - goto out; + goto error; } /* Is this a domain that was under libvirt control? */ if (libxl_userdata_retrieve(cfg->ctx, vm->def->id, "libvirt-xml", &data, &len)) { VIR_DEBUG("libxl_userdata_retrieve failed, ignoring domain %d", vm->def->id); - goto out; + goto error; } /* Update domid in case it changed (e.g. reboot) while we were gone? */ @@ -405,7 +407,7 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Update hostdev state */ if (virHostdevUpdateActiveDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, hostdev_flags) < 0) - goto out; + goto error; if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -413,21 +415,23 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); + cleanup: libxl_dominfo_dispose(&d_info); virObjectUnlock(vm); + virObjectUnref(vm); virObjectUnref(cfg); - return 0; + return ret; - out: - libxl_dominfo_dispose(&d_info); + error: libxlDomainCleanup(driver, vm); - if (!vm->persistent) + if (!vm->persistent) { virDomainObjListRemoveLocked(driver->domains, vm); - else - virObjectUnlock(vm); - virObjectUnref(cfg); - return -1; + /* virDomainObjListRemoveLocked leaves the object unlocked, + * lock it again to factorize more code. */ + virObjectLock(vm); + } + goto cleanup; } static void -- 2.6.6
In case of error, libxlReconnectDomain may call virDomainObjListRemoveLocked. However it has no local reference on the domain object, leading to segfault. Get a reference to the domain object at the start of the function and release it at the end to avoid problems.
This commit also factorizes code between the error and normal ends. Also noticed in the rest of the patches, but I think you forgot to include
--- src/libxl/libxl_driver.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index cb501cf..5008c00 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -373,11 +373,13 @@ libxlReconnectDomain(virDomainObjPtr vm, uint8_t *data = NULL; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; + int ret = -1;
#ifdef LIBXL_HAVE_PVUSB hostdev_flags |= VIR_HOSTDEV_SP_USB; #endif
+ virObjectRef(vm); virObjectLock(vm);
libxl_dominfo_init(&d_info); @@ -385,18 +387,18 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Does domain still exist? */ rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id); if (rc == ERROR_INVAL) { - goto out; + goto error; } else if (rc != 0) { VIR_DEBUG("libxl_domain_info failed (code %d), ignoring domain %d", rc, vm->def->id); - goto out; + goto error; }
/* Is this a domain that was under libvirt control? */ if (libxl_userdata_retrieve(cfg->ctx, vm->def->id, "libvirt-xml", &data, &len)) { VIR_DEBUG("libxl_userdata_retrieve failed, ignoring domain %d", vm->def->id); - goto out; + goto error; }
/* Update domid in case it changed (e.g. reboot) while we were gone? */ @@ -405,7 +407,7 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Update hostdev state */ if (virHostdevUpdateActiveDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, hostdev_flags) < 0) - goto out; + goto error;
if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -413,21 +415,23 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); After this patch it will always returns an error. Although
On 07/27/2016 02:42 PM, Cédric Bosdonnat wrote: the SoB tag (Signed-off-by). patch 3 of this series does add the "ret = 0" here. I think it should a part of this patch.
+ cleanup: libxl_dominfo_dispose(&d_info); virObjectUnlock(vm); + virObjectUnref(vm); virObjectUnref(cfg); - return 0; + return ret;
- out: - libxl_dominfo_dispose(&d_info); + error: libxlDomainCleanup(driver, vm); - if (!vm->persistent) + if (!vm->persistent) { virDomainObjListRemoveLocked(driver->domains, vm); - else - virObjectUnlock(vm); - virObjectUnref(cfg);
- return -1; + /* virDomainObjListRemoveLocked leaves the object unlocked, + * lock it again to factorize more code. */ + virObjectLock(vm); + } + goto cleanup; }
static void
On Wed, 2016-07-27 at 17:40 +0100, Joao Martins wrote:
This commit also factorizes code between the error and normal ends. Also noticed in the rest of the patches, but I think you forgot to include the SoB tag (Signed-off-by).
I never include it and it's not mandatory here in the libvirt community.
--- src/libxl/libxl_driver.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index cb501cf..5008c00 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -373,11 +373,13 @@ libxlReconnectDomain(virDomainObjPtr vm, uint8_t *data = NULL; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; + int ret = -1;
#ifdef LIBXL_HAVE_PVUSB hostdev_flags |= VIR_HOSTDEV_SP_USB; #endif
+ virObjectRef(vm); virObjectLock(vm);
libxl_dominfo_init(&d_info); @@ -385,18 +387,18 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Does domain still exist? */ rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id); if (rc == ERROR_INVAL) { - goto out; + goto error; } else if (rc != 0) { VIR_DEBUG("libxl_domain_info failed (code %d), ignoring domain %d", rc, vm->def->id); - goto out; + goto error; }
/* Is this a domain that was under libvirt control? */ if (libxl_userdata_retrieve(cfg->ctx, vm->def->id, "libvirt-xml", &data, &len)) { VIR_DEBUG("libxl_userdata_retrieve failed, ignoring domain %d", vm->def->id); - goto out; + goto error; }
/* Update domid in case it changed (e.g. reboot) while we were gone? */ @@ -405,7 +407,7 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Update hostdev state */ if (virHostdevUpdateActiveDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, hostdev_flags) < 0) - goto out; + goto error;
if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -413,21 +415,23 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); After this patch it will always returns an error. Although patch 3 of this series does add the "ret = 0" here. I think it should a part of this patch.
Indeed, I failed splitting the change correctly, nice catch! -- Cedric
+ cleanup: libxl_dominfo_dispose(&d_info); virObjectUnlock(vm); + virObjectUnref(vm); virObjectUnref(cfg); - return 0; + return ret;
- out: - libxl_dominfo_dispose(&d_info); + error: libxlDomainCleanup(driver, vm); - if (!vm->persistent) + if (!vm->persistent) { virDomainObjListRemoveLocked(driver->domains, vm); - else - virObjectUnlock(vm); - virObjectUnref(cfg);
- return -1; + /* virDomainObjListRemoveLocked leaves the object unlocked, + * lock it again to factorize more code. */ + virObjectLock(vm); + } + goto cleanup; }
static void
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Introduce libxl hook and use it for start, prepare, started, stop, stopped, migrate events. --- docs/hooks.html.in | 53 ++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_driver.c | 24 +++++++++++++++ src/libxl/libxl_migration.c | 58 +++++++++++++++++++++++++++++++++++ src/util/virhook.c | 16 +++++++++- src/util/virhook.h | 13 ++++++++ 6 files changed, 236 insertions(+), 3 deletions(-) diff --git a/docs/hooks.html.in b/docs/hooks.html.in index d4f4ac3..11073cb 100644 --- a/docs/hooks.html.in +++ b/docs/hooks.html.in @@ -17,8 +17,10 @@ (<span class="since">since 0.8.0</span>)<br/><br/></li> <li>A QEMU guest is started or stopped (<span class="since">since 0.8.0</span>)<br/><br/></li> - <li>An LXC guest is started or stopped + <li>An LXC guest is started or stopped (<span class="since">since 0.8.0</span>)<br/><br/></li> + <li>A libxl-handled Xen guest is started or stopped + (<span class="since">since 2.1.0</span>)<br/><br/></li> <li>A network is started or stopped or an interface is plugged/unplugged to/from the network (<span class="since">since 1.2.2</span>)<br/><br/></li> @@ -41,7 +43,7 @@ <br/> <h2><a name="names">Script names</a></h2> - <p>At present, there are three hook scripts that can be called:</p> + <p>At present, there are five hook scripts that can be called:</p> <ul> <li><code>/etc/libvirt/hooks/daemon</code><br/><br/> Executed when the libvirt daemon is started, stopped, or reloads @@ -50,6 +52,9 @@ Executed when a QEMU guest is started, stopped, or migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/lxc</code><br /><br/> Executed when an LXC guest is started or stopped</li> + <li><code>/etc/libvirt/hooks/libxl</code><br/><br/> + Executed when a libxl-handled Xen guest is started, stopped, or + migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/network</code><br/><br/> Executed when a network is started or stopped or an interface is plugged/unplugged to/from the network</li> @@ -235,6 +240,50 @@ </li> </ul> + <h5><a name="libxl">/etc/libvirt/hooks/libxl</a></h5> + <ul> + <li>Before a Xen guest is started using libxl driver, the libxl hook + script is called in three locations; if any location fails, the guest + is not started. The first location, <span class="since">since + 2.1.0</span>, is before libvirt performs any resource + labeling, and the hook can allocate resources not managed by + libvirt. This is called as:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name prepare begin -</pre> + The second location, available <span class="since">Since + 2.1.0</span>, occurs after libvirt has finished labeling + all resources, but has not yet started the guest, called as:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name start begin -</pre> + The third location, <span class="since">2.1.0</span>, + occurs after the domain has successfully started up:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name started begin -</pre> + </li> + <li>When a libxl-handled Xen guest is stopped, the libxl hook script + is called in two locations, to match the startup. + First, <span class="since">since 2.1.0</span>, the hook is + called before libvirt restores any labels:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name stopped end -</pre> + Then, after libvirt has released all resources, the hook is + called again, <span class="since">since 2.1.0</span>, to allow + any additional resource cleanup:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name release end -</pre></li> + <li><span class="since">Since 2.1.0</span>, the libxl hook script + is also called at the beginning of incoming migration. It is called + as: <pre>/etc/libvirt/hooks/libxl guest_name migrate begin -</pre> + with domain XML sent to standard input of the script. In this case, + the script acts as a filter and is supposed to modify the domain + XML and print it out on its standard output. Empty output is + identical to copying the input XML without changing it. In case the + script returns failure or the output XML is not valid, incoming + migration will be canceled. This hook may be used, e.g., to change + location of disk images for incoming domains.</li> + <li><span class="since">Since 2.1.0</span>, the libxl hook script + is also called when the libvirtd daemon restarts and reconnects + to previously running Xen domains. If the script fails, the + existing Xen domains will be killed off. It is called as: + <pre>/etc/libvirt/hooks/libxl guest_name reconnect begin -</pre> + </li> + </ul> + <h5><a name="network">/etc/libvirt/hooks/network</a></h5> <ul> <li><span class="since">Since 1.2.2</span>, before a network is started, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 886b40f..d04dc5e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -32,6 +32,7 @@ #include "viratomic.h" #include "virfile.h" #include "virerror.h" +#include "virhook.h" #include "virlog.h" #include "virstring.h" #include "virtime.h" @@ -737,6 +738,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, hostdev_flags |= VIR_HOSTDEV_SP_USB; #endif + /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL); + VIR_FREE(xml); + } + virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, hostdev_flags, NULL); @@ -788,6 +800,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, VIR_FREE(file); } + /* The "release" hook cleans up additional resources */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_RELEASE, VIR_HOOK_SUBOP_END, + NULL, xml, NULL); + VIR_FREE(xml); + } + if (vm->newDef) { virDomainDefFree(vm->def); vm->def = vm->newDef; @@ -1107,6 +1130,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm) < 0) goto cleanup; + /* Run an early hook to set-up missing devices */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + if (virDomainLockProcessStart(driver->lockManager, "xen:///system", vm, @@ -1135,6 +1175,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def, hostdev_flags) < 0) goto cleanup_dom; + /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_START, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + if (priv->hookRun) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); @@ -1143,6 +1200,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def->id, vm->def->name, uuidstr); + } /* Unlock virDomainObj while creating the domain */ @@ -1215,6 +1273,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); + /* finally we can call the 'started' hook script if any */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_STARTED, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, restore_fd < 0 ? VIR_DOMAIN_EVENT_STARTED_BOOTED : diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5008c00..f500892 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -42,6 +42,7 @@ #include "virfile.h" #include "viralloc.h" #include "viruuid.h" +#include "virhook.h" #include "vircommand.h" #include "libxl_domain.h" #include "libxl_driver.h" @@ -415,6 +416,29 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); + /* now that we know it's reconnected call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL) && + STRNEQ("Domain-0", vm->def->name)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + /* we can't stop the operation even if the script raised an error */ + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + if (hookret < 0) { + /* Stop the domain if the hook failed */ + if (virDomainObjIsActive(vm)) { + libxlDomainDestroyInternal(driver, vm); + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + } + goto error; + } + } + + ret = 0; + cleanup: libxl_dominfo_dispose(&d_info); virObjectUnlock(vm); diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index bf285a4..1885d49 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -36,6 +36,7 @@ #include "virstring.h" #include "virobject.h" #include "virthread.h" +#include "virhook.h" #include "rpc/virnetsocket.h" #include "libxl_domain.h" #include "libxl_driver.h" @@ -490,9 +491,11 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, unsigned int flags) { libxlDriverPrivatePtr driver = dconn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); libxlMigrationCookiePtr mig = NULL; virDomainObjPtr vm = NULL; char *hostname = NULL; + char *xmlout = NULL; unsigned short port; char portstr[100]; virURIPtr uri = NULL; @@ -500,6 +503,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, size_t nsocks = 0; int nsocks_listen = 0; libxlMigrationDstArgs *args = NULL; + bool taint_hook = false; + libxlDomainObjPrivatePtr priv = NULL; size_t i; int ret = -1; @@ -513,6 +518,51 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, goto error; } + /* Let migration hook filter domain XML */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml; + int hookret; + + if (!(xml = virDomainDefFormat(*def, cfg->caps, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) + goto error; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name, + VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, &xmlout); + VIR_FREE(xml); + + if (hookret < 0) { + goto error; + } else if (hookret == 0) { + if (virStringIsEmpty(xmlout)) { + VIR_DEBUG("Migrate hook filter returned nothing; using the" + " original XML"); + } else { + virDomainDefPtr newdef; + + VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout); + newdef = virDomainDefParseString(xmlout, cfg->caps, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); + if (!newdef) + goto error; + + /* TODO At some stage we will want to have some check of what the user + * did in the hook. */ + + virDomainDefFree(*def); + *def = newdef; + /* We should taint the domain here. However, @vm and therefore + * privateData too are still NULL, so just notice the fact and + * taint it later. */ + taint_hook = true; + } + } + } + + if (!(vm = virDomainObjListAdd(driver->domains, *def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | @@ -520,6 +570,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, NULL))) goto error; *def = NULL; + priv = vm->privateData; + + if (taint_hook) { + /* Domain XML has been altered by a hook script. */ + priv->hookRun = true; + } /* Create socket connection to receive migration data */ if (!uri_in) { @@ -640,6 +696,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, } done: + VIR_FREE(xmlout); libxlMigrationCookieFree(mig); if (!uri_in) VIR_FREE(hostname); @@ -647,6 +704,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virURIFree(uri); if (vm) virObjectUnlock(vm); + virObjectUnref(cfg); return ret; } diff --git a/src/util/virhook.c b/src/util/virhook.c index a8422a2..facd74a 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -51,13 +51,15 @@ VIR_ENUM_DECL(virHookSubop) VIR_ENUM_DECL(virHookQemuOp) VIR_ENUM_DECL(virHookLxcOp) VIR_ENUM_DECL(virHookNetworkOp) +VIR_ENUM_DECL(virHookLibxlOp) VIR_ENUM_IMPL(virHookDriver, VIR_HOOK_DRIVER_LAST, "daemon", "qemu", "lxc", - "network") + "network", + "libxl") VIR_ENUM_IMPL(virHookDaemonOp, VIR_HOOK_DAEMON_OP_LAST, "start", @@ -96,6 +98,15 @@ VIR_ENUM_IMPL(virHookNetworkOp, VIR_HOOK_NETWORK_OP_LAST, "unplugged", "updated") +VIR_ENUM_IMPL(virHookLibxlOp, VIR_HOOK_LIBXL_OP_LAST, + "start", + "stopped", + "prepare", + "release", + "migrate", + "started", + "reconnect") + static int virHooksFound = -1; /** @@ -261,6 +272,9 @@ virHookCall(int driver, case VIR_HOOK_DRIVER_LXC: opstr = virHookLxcOpTypeToString(op); break; + case VIR_HOOK_DRIVER_LIBXL: + opstr = virHookLibxlOpTypeToString(op); + break; case VIR_HOOK_DRIVER_NETWORK: opstr = virHookNetworkOpTypeToString(op); } diff --git a/src/util/virhook.h b/src/util/virhook.h index 4015426..205249c 100644 --- a/src/util/virhook.h +++ b/src/util/virhook.h @@ -31,6 +31,7 @@ typedef enum { VIR_HOOK_DRIVER_QEMU, /* QEmu domains related events */ VIR_HOOK_DRIVER_LXC, /* LXC domains related events */ VIR_HOOK_DRIVER_NETWORK, /* network related events */ + VIR_HOOK_DRIVER_LIBXL, /* Xen libxl domains related events */ VIR_HOOK_DRIVER_LAST, } virHookDriverType; @@ -87,6 +88,18 @@ typedef enum { VIR_HOOK_NETWORK_OP_LAST, } virHookNetworkOpType; +typedef enum { + VIR_HOOK_LIBXL_OP_START, /* domain is about to start */ + VIR_HOOK_LIBXL_OP_STOPPED, /* domain has stopped */ + VIR_HOOK_LIBXL_OP_PREPARE, /* domain startup initiated */ + VIR_HOOK_LIBXL_OP_RELEASE, /* domain destruction is over */ + VIR_HOOK_LIBXL_OP_MIGRATE, /* domain is being migrated */ + VIR_HOOK_LIBXL_OP_STARTED, /* domain has started */ + VIR_HOOK_LIBXL_OP_RECONNECT, /* domain is being reconnected by libvirt */ + + VIR_HOOK_LIBXL_OP_LAST, +} virHookLibxlOpType; + int virHookInitialize(void); int virHookPresent(int driver); -- 2.6.6
On 07/27/2016 02:42 PM, Cédric Bosdonnat wrote:
Introduce libxl hook and use it for start, prepare, started, stop, stopped, migrate events. Looks good to me with one comment and few nits. Looking at lxc and qemu drivers virHook support seems to be similar so I am assuming this is correct. But this initial review has a grain of salt as I am not fully familiar (yet) with virHook support.
--- docs/hooks.html.in | 53 ++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_driver.c | 24 +++++++++++++++ src/libxl/libxl_migration.c | 58 +++++++++++++++++++++++++++++++++++ src/util/virhook.c | 16 +++++++++- src/util/virhook.h | 13 ++++++++ 6 files changed, 236 insertions(+), 3 deletions(-)
diff --git a/docs/hooks.html.in b/docs/hooks.html.in index d4f4ac3..11073cb 100644 --- a/docs/hooks.html.in +++ b/docs/hooks.html.in @@ -17,8 +17,10 @@ (<span class="since">since 0.8.0</span>)<br/><br/></li> <li>A QEMU guest is started or stopped (<span class="since">since 0.8.0</span>)<br/><br/></li> - <li>An LXC guest is started or stopped + <li>An LXC guest is started or stopped (<span class="since">since 0.8.0</span>)<br/><br/></li> + <li>A libxl-handled Xen guest is started or stopped + (<span class="since">since 2.1.0</span>)<br/><br/></li> <li>A network is started or stopped or an interface is plugged/unplugged to/from the network (<span class="since">since 1.2.2</span>)<br/><br/></li> @@ -41,7 +43,7 @@ <br/>
<h2><a name="names">Script names</a></h2> - <p>At present, there are three hook scripts that can be called:</p> + <p>At present, there are five hook scripts that can be called:</p> <ul> <li><code>/etc/libvirt/hooks/daemon</code><br/><br/> Executed when the libvirt daemon is started, stopped, or reloads @@ -50,6 +52,9 @@ Executed when a QEMU guest is started, stopped, or migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/lxc</code><br /><br/> Executed when an LXC guest is started or stopped</li> + <li><code>/etc/libvirt/hooks/libxl</code><br/><br/> + Executed when a libxl-handled Xen guest is started, stopped, or + migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/network</code><br/><br/> Executed when a network is started or stopped or an interface is plugged/unplugged to/from the network</li> @@ -235,6 +240,50 @@ </li> </ul>
+ <h5><a name="libxl">/etc/libvirt/hooks/libxl</a></h5> + <ul> + <li>Before a Xen guest is started using libxl driver, the libxl hook + script is called in three locations; if any location fails, the guest + is not started. The first location, <span class="since">since + 2.1.0</span>, is before libvirt performs any resource + labeling, and the hook can allocate resources not managed by + libvirt. This is called as:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name prepare begin -</pre> + The second location, available <span class="since">Since + 2.1.0</span>, occurs after libvirt has finished labeling + all resources, but has not yet started the guest, called as:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name start begin -</pre> + The third location, <span class="since">2.1.0</span>, + occurs after the domain has successfully started up:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name started begin -</pre> + </li> + <li>When a libxl-handled Xen guest is stopped, the libxl hook script + is called in two locations, to match the startup. + First, <span class="since">since 2.1.0</span>, the hook is + called before libvirt restores any labels:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name stopped end -</pre> + Then, after libvirt has released all resources, the hook is + called again, <span class="since">since 2.1.0</span>, to allow + any additional resource cleanup:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name release end -</pre></li> + <li><span class="since">Since 2.1.0</span>, the libxl hook script + is also called at the beginning of incoming migration. It is called + as: <pre>/etc/libvirt/hooks/libxl guest_name migrate begin -</pre> + with domain XML sent to standard input of the script. In this case, + the script acts as a filter and is supposed to modify the domain + XML and print it out on its standard output. Empty output is + identical to copying the input XML without changing it. In case the + script returns failure or the output XML is not valid, incoming + migration will be canceled. This hook may be used, e.g., to change + location of disk images for incoming domains.</li> + <li><span class="since">Since 2.1.0</span>, the libxl hook script + is also called when the libvirtd daemon restarts and reconnects + to previously running Xen domains. If the script fails, the + existing Xen domains will be killed off. It is called as: + <pre>/etc/libvirt/hooks/libxl guest_name reconnect begin -</pre> + </li> + </ul> + Not sure what's the norm for documentation patches on libvirt, but I still leave the suggestion of making docs part on a separate patch.
<h5><a name="network">/etc/libvirt/hooks/network</a></h5> <ul> <li><span class="since">Since 1.2.2</span>, before a network is started, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 886b40f..d04dc5e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -32,6 +32,7 @@ #include "viratomic.h" #include "virfile.h" #include "virerror.h" +#include "virhook.h" #include "virlog.h" #include "virstring.h" #include "virtime.h" @@ -737,6 +738,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, hostdev_flags |= VIR_HOSTDEV_SP_USB; #endif
+ /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL);
Shouldn't ignore_value(...) be around the virHookCall since we are ignoring the return value?
+ VIR_FREE(xml); + } + virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, hostdev_flags, NULL);
@@ -788,6 +800,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, VIR_FREE(file); }
+ /* The "release" hook cleans up additional resources */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_RELEASE, VIR_HOOK_SUBOP_END, + NULL, xml, NULL); Same here?
+ VIR_FREE(xml); + } + if (vm->newDef) { virDomainDefFree(vm->def); vm->def = vm->newDef; @@ -1107,6 +1130,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm) < 0) goto cleanup;
+ /* Run an early hook to set-up missing devices */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + if (virDomainLockProcessStart(driver->lockManager, "xen:///system", vm, @@ -1135,6 +1175,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def, hostdev_flags) < 0) goto cleanup_dom;
+ /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_START, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + if (priv->hookRun) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); @@ -1143,6 +1200,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def->id, vm->def->name, uuidstr); + Spurious whitespace.
}
/* Unlock virDomainObj while creating the domain */ @@ -1215,6 +1273,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque);
+ /* finally we can call the 'started' hook script if any */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_STARTED, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, restore_fd < 0 ? VIR_DOMAIN_EVENT_STARTED_BOOTED : diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5008c00..f500892 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -42,6 +42,7 @@ #include "virfile.h" #include "viralloc.h" #include "viruuid.h" +#include "virhook.h" #include "vircommand.h" #include "libxl_domain.h" #include "libxl_driver.h" @@ -415,6 +416,29 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW);
+ /* now that we know it's reconnected call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL) && + STRNEQ("Domain-0", vm->def->name)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + /* we can't stop the operation even if the script raised an error */ + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + if (hookret < 0) { + /* Stop the domain if the hook failed */ + if (virDomainObjIsActive(vm)) { + libxlDomainDestroyInternal(driver, vm); + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + } + goto error; + } + } + + ret = 0;
As mentioned in Patch 2, I think this needs to be a part of patch 2.
+ cleanup: libxl_dominfo_dispose(&d_info); virObjectUnlock(vm); diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index bf285a4..1885d49 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -36,6 +36,7 @@ #include "virstring.h" #include "virobject.h" #include "virthread.h" +#include "virhook.h" #include "rpc/virnetsocket.h" #include "libxl_domain.h" #include "libxl_driver.h" @@ -490,9 +491,11 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, unsigned int flags) { libxlDriverPrivatePtr driver = dconn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); libxlMigrationCookiePtr mig = NULL; virDomainObjPtr vm = NULL; char *hostname = NULL; + char *xmlout = NULL; unsigned short port; char portstr[100]; virURIPtr uri = NULL; @@ -500,6 +503,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, size_t nsocks = 0; int nsocks_listen = 0; libxlMigrationDstArgs *args = NULL; + bool taint_hook = false; + libxlDomainObjPrivatePtr priv = NULL; size_t i; int ret = -1;
@@ -513,6 +518,51 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, goto error; }
+ /* Let migration hook filter domain XML */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml; + int hookret; + + if (!(xml = virDomainDefFormat(*def, cfg->caps, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) + goto error; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name, + VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, &xmlout); + VIR_FREE(xml); + + if (hookret < 0) { + goto error; + } else if (hookret == 0) { + if (virStringIsEmpty(xmlout)) { + VIR_DEBUG("Migrate hook filter returned nothing; using the" + " original XML"); + } else { + virDomainDefPtr newdef; + + VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout); + newdef = virDomainDefParseString(xmlout, cfg->caps, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); + if (!newdef) + goto error; + + /* TODO At some stage we will want to have some check of what the user + * did in the hook. */ + + virDomainDefFree(*def); + *def = newdef; + /* We should taint the domain here. However, @vm and therefore + * privateData too are still NULL, so just notice the fact and + * taint it later. */ + taint_hook = true; + } + } + } + + Probably one newline is enough here instead of two after the closing bracket.
if (!(vm = virDomainObjListAdd(driver->domains, *def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | @@ -520,6 +570,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, NULL))) goto error; *def = NULL; + priv = vm->privateData; + + if (taint_hook) { + /* Domain XML has been altered by a hook script. */ + priv->hookRun = true; + }
/* Create socket connection to receive migration data */ if (!uri_in) { @@ -640,6 +696,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, }
done: + VIR_FREE(xmlout); libxlMigrationCookieFree(mig); if (!uri_in) VIR_FREE(hostname); @@ -647,6 +704,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virURIFree(uri); if (vm) virObjectUnlock(vm); + virObjectUnref(cfg); return ret; }
diff --git a/src/util/virhook.c b/src/util/virhook.c index a8422a2..facd74a 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -51,13 +51,15 @@ VIR_ENUM_DECL(virHookSubop) VIR_ENUM_DECL(virHookQemuOp) VIR_ENUM_DECL(virHookLxcOp) VIR_ENUM_DECL(virHookNetworkOp) +VIR_ENUM_DECL(virHookLibxlOp)
VIR_ENUM_IMPL(virHookDriver, VIR_HOOK_DRIVER_LAST, "daemon", "qemu", "lxc", - "network") + "network", + "libxl")
VIR_ENUM_IMPL(virHookDaemonOp, VIR_HOOK_DAEMON_OP_LAST, "start", @@ -96,6 +98,15 @@ VIR_ENUM_IMPL(virHookNetworkOp, VIR_HOOK_NETWORK_OP_LAST, "unplugged", "updated")
+VIR_ENUM_IMPL(virHookLibxlOp, VIR_HOOK_LIBXL_OP_LAST, + "start", + "stopped", + "prepare", + "release", + "migrate", + "started", + "reconnect") + static int virHooksFound = -1;
/** @@ -261,6 +272,9 @@ virHookCall(int driver, case VIR_HOOK_DRIVER_LXC: opstr = virHookLxcOpTypeToString(op); break; + case VIR_HOOK_DRIVER_LIBXL: + opstr = virHookLibxlOpTypeToString(op); + break; case VIR_HOOK_DRIVER_NETWORK: opstr = virHookNetworkOpTypeToString(op); } diff --git a/src/util/virhook.h b/src/util/virhook.h index 4015426..205249c 100644 --- a/src/util/virhook.h +++ b/src/util/virhook.h @@ -31,6 +31,7 @@ typedef enum { VIR_HOOK_DRIVER_QEMU, /* QEmu domains related events */ VIR_HOOK_DRIVER_LXC, /* LXC domains related events */ VIR_HOOK_DRIVER_NETWORK, /* network related events */ + VIR_HOOK_DRIVER_LIBXL, /* Xen libxl domains related events */
VIR_HOOK_DRIVER_LAST, } virHookDriverType; @@ -87,6 +88,18 @@ typedef enum { VIR_HOOK_NETWORK_OP_LAST, } virHookNetworkOpType;
+typedef enum { + VIR_HOOK_LIBXL_OP_START, /* domain is about to start */ + VIR_HOOK_LIBXL_OP_STOPPED, /* domain has stopped */ + VIR_HOOK_LIBXL_OP_PREPARE, /* domain startup initiated */ + VIR_HOOK_LIBXL_OP_RELEASE, /* domain destruction is over */ + VIR_HOOK_LIBXL_OP_MIGRATE, /* domain is being migrated */ + VIR_HOOK_LIBXL_OP_STARTED, /* domain has started */ + VIR_HOOK_LIBXL_OP_RECONNECT, /* domain is being reconnected by libvirt */ + + VIR_HOOK_LIBXL_OP_LAST, +} virHookLibxlOpType; + int virHookInitialize(void);
int virHookPresent(int driver);
On Wed, 2016-07-27 at 17:40 +0100, Joao Martins wrote:
On 07/27/2016 02:42 PM, Cédric Bosdonnat wrote:
Introduce libxl hook and use it for start, prepare, started, stop, stopped, migrate events. Looks good to me with one comment and few nits. Looking at lxc and qemu drivers virHook support seems to be similar so I am assuming this is correct. But this initial review has a grain of salt as I am not fully familiar (yet) with virHook support.
--- docs/hooks.html.in | 53 ++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_driver.c | 24 +++++++++++++++ src/libxl/libxl_migration.c | 58 +++++++++++++++++++++++++++++++++++ src/util/virhook.c | 16 +++++++++- src/util/virhook.h | 13 ++++++++ 6 files changed, 236 insertions(+), 3 deletions(-)
diff --git a/docs/hooks.html.in b/docs/hooks.html.in index d4f4ac3..11073cb 100644 --- a/docs/hooks.html.in +++ b/docs/hooks.html.in @@ -17,8 +17,10 @@ (<span class="since">since 0.8.0</span>)<br/><br/></li> <li>A QEMU guest is started or stopped (<span class="since">since 0.8.0</span>)<br/><br/></li> - <li>An LXC guest is started or stopped + <li>An LXC guest is started or stopped (<span class="since">since 0.8.0</span>)<br/><br/></li> + <li>A libxl-handled Xen guest is started or stopped + (<span class="since">since 2.1.0</span>)<br/><br/></li> <li>A network is started or stopped or an interface is plugged/unplugged to/from the network (<span class="since">since 1.2.2</span>)<br/><br/></li> @@ -41,7 +43,7 @@ <br/>
<h2><a name="names">Script names</a></h2> - <p>At present, there are three hook scripts that can be called:</p> + <p>At present, there are five hook scripts that can be called:</p> <ul> <li><code>/etc/libvirt/hooks/daemon</code><br/><br/> Executed when the libvirt daemon is started, stopped, or reloads @@ -50,6 +52,9 @@ Executed when a QEMU guest is started, stopped, or migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/lxc</code><br /><br/> Executed when an LXC guest is started or stopped</li> + <li><code>/etc/libvirt/hooks/libxl</code><br/><br/> + Executed when a libxl-handled Xen guest is started, stopped, or + migrated<br/><br/></li> <li><code>/etc/libvirt/hooks/network</code><br/><br/> Executed when a network is started or stopped or an interface is plugged/unplugged to/from the network</li> @@ -235,6 +240,50 @@ </li> </ul>
+ <h5><a name="libxl">/etc/libvirt/hooks/libxl</a></h5> + <ul> + <li>Before a Xen guest is started using libxl driver, the libxl hook + script is called in three locations; if any location fails, the guest + is not started. The first location, <span class="since">since + 2.1.0</span>, is before libvirt performs any resource + labeling, and the hook can allocate resources not managed by + libvirt. This is called as:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name prepare begin -</pre> + The second location, available <span class="since">Since + 2.1.0</span>, occurs after libvirt has finished labeling + all resources, but has not yet started the guest, called as:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name start begin -</pre> + The third location, <span class="since">2.1.0</span>, + occurs after the domain has successfully started up:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name started begin -</pre> + </li> + <li>When a libxl-handled Xen guest is stopped, the libxl hook script + is called in two locations, to match the startup. + First, <span class="since">since 2.1.0</span>, the hook is + called before libvirt restores any labels:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name stopped end -</pre> + Then, after libvirt has released all resources, the hook is + called again, <span class="since">since 2.1.0</span>, to allow + any additional resource cleanup:<br/> + <pre>/etc/libvirt/hooks/libxl guest_name release end -</pre></li> + <li><span class="since">Since 2.1.0</span>, the libxl hook script + is also called at the beginning of incoming migration. It is called + as: <pre>/etc/libvirt/hooks/libxl guest_name migrate begin -</pre> + with domain XML sent to standard input of the script. In this case, + the script acts as a filter and is supposed to modify the domain + XML and print it out on its standard output. Empty output is + identical to copying the input XML without changing it. In case the + script returns failure or the output XML is not valid, incoming + migration will be canceled. This hook may be used, e.g., to change + location of disk images for incoming domains.</li> + <li><span class="since">Since 2.1.0</span>, the libxl hook script + is also called when the libvirtd daemon restarts and reconnects + to previously running Xen domains. If the script fails, the + existing Xen domains will be killed off. It is called as: + <pre>/etc/libvirt/hooks/libxl guest_name reconnect begin -</pre> + </li> + </ul> + Not sure what's the norm for documentation patches on libvirt, but I still leave the suggestion of making docs part on a separate patch.
Usually we have doc within the commit introducing the feature. git log docs/hooks.html.in can confirm it.
<h5><a name="network">/etc/libvirt/hooks/network</a></h5> <ul> <li><span class="since">Since 1.2.2</span>, before a network is started, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 886b40f..d04dc5e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -32,6 +32,7 @@ #include "viratomic.h" #include "virfile.h" #include "virerror.h" +#include "virhook.h" #include "virlog.h" #include "virstring.h" #include "virtime.h" @@ -737,6 +738,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, hostdev_flags |= VIR_HOSTDEV_SP_USB; #endif
+ /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL);
Shouldn't ignore_value(...) be around the virHookCall since we are ignoring the return value?
Indeed, it's used in the qemu driver, but not in the lxc one... I'll add it. I'll resubmit with your comments addressed. Thanks for the review. -- Cedric
+ VIR_FREE(xml); + } + virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, hostdev_flags, NULL);
@@ -788,6 +800,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, VIR_FREE(file); }
+ /* The "release" hook cleans up additional resources */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + + /* we can't stop the operation even if the script raised an error */ + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_RELEASE, VIR_HOOK_SUBOP_END, + NULL, xml, NULL); Same here?
+ VIR_FREE(xml); + } + if (vm->newDef) { virDomainDefFree(vm->def); vm->def = vm->newDef; @@ -1107,6 +1130,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm) < 0) goto cleanup;
+ /* Run an early hook to set-up missing devices */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + if (virDomainLockProcessStart(driver->lockManager, "xen:///system", vm, @@ -1135,6 +1175,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def, hostdev_flags) < 0) goto cleanup_dom;
+ /* now that we know it is about to start call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_START, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + if (priv->hookRun) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); @@ -1143,6 +1200,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, vm->def->id, vm->def->name, uuidstr); + Spurious whitespace.
}
/* Unlock virDomainObj while creating the domain */ @@ -1215,6 +1273,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque);
+ /* finally we can call the 'started' hook script if any */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_STARTED, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto cleanup_dom; + } + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, restore_fd < 0 ? VIR_DOMAIN_EVENT_STARTED_BOOTED : diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5008c00..f500892 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -42,6 +42,7 @@ #include "virfile.h" #include "viralloc.h" #include "viruuid.h" +#include "virhook.h" #include "vircommand.h" #include "libxl_domain.h" #include "libxl_driver.h" @@ -415,6 +416,29 @@ libxlReconnectDomain(virDomainObjPtr vm, /* Enable domain death events */ libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW);
+ /* now that we know it's reconnected call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL) && + STRNEQ("Domain-0", vm->def->name)) { + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); + int hookret; + + /* we can't stop the operation even if the script raised an error */ + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + if (hookret < 0) { + /* Stop the domain if the hook failed */ + if (virDomainObjIsActive(vm)) { + libxlDomainDestroyInternal(driver, vm); + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + } + goto error; + } + } + + ret = 0;
As mentioned in Patch 2, I think this needs to be a part of patch 2.
+ cleanup: libxl_dominfo_dispose(&d_info); virObjectUnlock(vm); diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index bf285a4..1885d49 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -36,6 +36,7 @@ #include "virstring.h" #include "virobject.h" #include "virthread.h" +#include "virhook.h" #include "rpc/virnetsocket.h" #include "libxl_domain.h" #include "libxl_driver.h" @@ -490,9 +491,11 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, unsigned int flags) { libxlDriverPrivatePtr driver = dconn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); libxlMigrationCookiePtr mig = NULL; virDomainObjPtr vm = NULL; char *hostname = NULL; + char *xmlout = NULL; unsigned short port; char portstr[100]; virURIPtr uri = NULL; @@ -500,6 +503,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, size_t nsocks = 0; int nsocks_listen = 0; libxlMigrationDstArgs *args = NULL; + bool taint_hook = false; + libxlDomainObjPrivatePtr priv = NULL; size_t i; int ret = -1;
@@ -513,6 +518,51 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, goto error; }
+ /* Let migration hook filter domain XML */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + char *xml; + int hookret; + + if (!(xml = virDomainDefFormat(*def, cfg->caps, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) + goto error; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name, + VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, &xmlout); + VIR_FREE(xml); + + if (hookret < 0) { + goto error; + } else if (hookret == 0) { + if (virStringIsEmpty(xmlout)) { + VIR_DEBUG("Migrate hook filter returned nothing; using the" + " original XML"); + } else { + virDomainDefPtr newdef; + + VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout); + newdef = virDomainDefParseString(xmlout, cfg->caps, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); + if (!newdef) + goto error; + + /* TODO At some stage we will want to have some check of what the user + * did in the hook. */ + + virDomainDefFree(*def); + *def = newdef; + /* We should taint the domain here. However, @vm and therefore + * privateData too are still NULL, so just notice the fact and + * taint it later. */ + taint_hook = true; + } + } + } + + Probably one newline is enough here instead of two after the closing bracket.
if (!(vm = virDomainObjListAdd(driver->domains, *def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | @@ -520,6 +570,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, NULL))) goto error; *def = NULL; + priv = vm->privateData; + + if (taint_hook) { + /* Domain XML has been altered by a hook script. */ + priv->hookRun = true; + }
/* Create socket connection to receive migration data */ if (!uri_in) { @@ -640,6 +696,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, }
done: + VIR_FREE(xmlout); libxlMigrationCookieFree(mig); if (!uri_in) VIR_FREE(hostname); @@ -647,6 +704,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, virURIFree(uri); if (vm) virObjectUnlock(vm); + virObjectUnref(cfg); return ret; }
diff --git a/src/util/virhook.c b/src/util/virhook.c index a8422a2..facd74a 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -51,13 +51,15 @@ VIR_ENUM_DECL(virHookSubop) VIR_ENUM_DECL(virHookQemuOp) VIR_ENUM_DECL(virHookLxcOp) VIR_ENUM_DECL(virHookNetworkOp) +VIR_ENUM_DECL(virHookLibxlOp)
VIR_ENUM_IMPL(virHookDriver, VIR_HOOK_DRIVER_LAST, "daemon", "qemu", "lxc", - "network") + "network", + "libxl")
VIR_ENUM_IMPL(virHookDaemonOp, VIR_HOOK_DAEMON_OP_LAST, "start", @@ -96,6 +98,15 @@ VIR_ENUM_IMPL(virHookNetworkOp, VIR_HOOK_NETWORK_OP_LAST, "unplugged", "updated")
+VIR_ENUM_IMPL(virHookLibxlOp, VIR_HOOK_LIBXL_OP_LAST, + "start", + "stopped", + "prepare", + "release", + "migrate", + "started", + "reconnect") + static int virHooksFound = -1;
/** @@ -261,6 +272,9 @@ virHookCall(int driver, case VIR_HOOK_DRIVER_LXC: opstr = virHookLxcOpTypeToString(op); break; + case VIR_HOOK_DRIVER_LIBXL: + opstr = virHookLibxlOpTypeToString(op); + break; case VIR_HOOK_DRIVER_NETWORK: opstr = virHookNetworkOpTypeToString(op); } diff --git a/src/util/virhook.h b/src/util/virhook.h index 4015426..205249c 100644 --- a/src/util/virhook.h +++ b/src/util/virhook.h @@ -31,6 +31,7 @@ typedef enum { VIR_HOOK_DRIVER_QEMU, /* QEmu domains related events */ VIR_HOOK_DRIVER_LXC, /* LXC domains related events */ VIR_HOOK_DRIVER_NETWORK, /* network related events */ + VIR_HOOK_DRIVER_LIBXL, /* Xen libxl domains related events */
VIR_HOOK_DRIVER_LAST, } virHookDriverType; @@ -87,6 +88,18 @@ typedef enum { VIR_HOOK_NETWORK_OP_LAST, } virHookNetworkOpType;
+typedef enum { + VIR_HOOK_LIBXL_OP_START, /* domain is about to start */ + VIR_HOOK_LIBXL_OP_STOPPED, /* domain has stopped */ + VIR_HOOK_LIBXL_OP_PREPARE, /* domain startup initiated */ + VIR_HOOK_LIBXL_OP_RELEASE, /* domain destruction is over */ + VIR_HOOK_LIBXL_OP_MIGRATE, /* domain is being migrated */ + VIR_HOOK_LIBXL_OP_STARTED, /* domain has started */ + VIR_HOOK_LIBXL_OP_RECONNECT, /* domain is being reconnected by libvirt */ + + VIR_HOOK_LIBXL_OP_LAST, +} virHookLibxlOpType; + int virHookInitialize(void);
int virHookPresent(int driver);
participants (3)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
Joao Martins