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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list