[libvirt] [PATCH 0/7] Add support for lock failure action

When sanlock is used as a locking driver and sanlock deamon loses access to its lockspace, it automatically kills off the domains that had their locks stored there. Apparently some management apps would like to change this behavior by configuring what should happen when locks are lost. Everything except the sanlock_helper code from patch 7/7 should be stable. The helper code may need some additional work (perhaps even touching other parts of libvirtd internals) because some lock failure action does not work as expected. For example, when automatic disk locks are enabled, the "pause" action will result in libvirtd calling back to sanlock and since it doesn't have access to the lockspace, the call would just hang. However, since currently the only requestor for this feature is VDSM and it doesn't use automatic disk locks, the sanlock_helper could work for them without any modifications. And since current sanlock_helper implementation is supposed to be changed when libvirtd gains administrative interface (see patch 7/7 for more details) we may even just live with its limitations for the time being. Jiri Denemark (7): conf: Generalize life cycle actions to event actions conf: Add on_lockfailure event configuration qemu: Use macro instead of "qemu" in the context of URI scheme locking: Add const char * parameter to avoid ugly typecasts locking: Pass hypervisor driver name when acquiring locks locking: Add support for lock failure action locking: Implement lock failure action in sanlock driver docs/formatdomain.html.in | 37 +++++-- docs/internals/locking.html.in | 8 ++ docs/schemas/domaincommon.rng | 30 +++++- libvirt.spec.in | 1 + src/Makefile.am | 13 ++- src/conf/domain_conf.c | 85 +++++++++------ src/conf/domain_conf.h | 18 +++- src/libvirt_private.syms | 2 + src/locking/domain_lock.c | 36 ++++--- src/locking/domain_lock.h | 4 + src/locking/lock_driver.h | 8 +- src/locking/lock_driver_nop.c | 1 + src/locking/lock_driver_sanlock.c | 114 +++++++++++++++++--- src/locking/lock_manager.c | 10 +- src/locking/lock_manager.h | 1 + src/locking/sanlock_helper.c | 214 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 21 ++-- src/qemu/qemu_hotplug.c | 15 ++- src/qemu/qemu_process.c | 4 +- 20 files changed, 537 insertions(+), 87 deletions(-) create mode 100644 src/locking/sanlock_helper.c -- 1.7.12

While current on_{poweroff,reboot,crash} action configuration is about configuring life cycle actions, they can all be considered events and actions that need to be done on a particular event. Let's generalize the code this way so that it can be reused later for non-lifecycle events. --- docs/formatdomain.html.in | 15 ++++++---- docs/schemas/domaincommon.rng | 8 +++--- src/conf/domain_conf.c | 66 +++++++++++++++++++++++-------------------- src/conf/domain_conf.h | 4 +-- 4 files changed, 51 insertions(+), 42 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bc4cc4a..57444fb 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -907,15 +907,11 @@ This guest NUMA specification is currently available only for QEMU/KVM. </p> - <h3><a name="elementsLifecycle">Lifecycle control</a></h3> + <h3><a name="elementsEvents">Events configuration</a></h3> <p> It is sometimes necessary to override the default actions taken - when a guest OS triggers a lifecycle operation. The following - collections of elements allow the actions to be specified. A - common use case is to force a reboot to be treated as a poweroff - when doing the initial OS installation. This allows the VM to be - re-configured for the first post-install bootup. + on various events. </p> <pre> @@ -925,6 +921,13 @@ <on_crash>restart</on_crash> ...</pre> + <p> + The following collections of elements allow the actions to be + specified when a guest OS triggers a lifecycle operation. A + common use case is to force a reboot to be treated as a poweroff + when doing the initial OS installation. This allows the VM to be + re-configured for the first post-install bootup. + </p> <dl> <dt><code>on_poweroff</code></dt> <dd>The content of this element specifies the action to take when diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..ab8d4a1 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -51,7 +51,7 @@ <ref name="clock"/> <ref name="resources"/> <ref name="features"/> - <ref name="termination"/> + <ref name="events"/> <optional> <ref name="pm"/> </optional> @@ -2226,10 +2226,10 @@ </element> </define> <!-- - When a domain terminates multiple policies can be applied depending - on how it ended: + When a certain event happens, multiple policies can be applied + depends on what happened: --> - <define name="termination"> + <define name="events"> <interleave> <optional> <element name="on_reboot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..40abf39 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7518,11 +7518,12 @@ error: return NULL; } -static int virDomainLifecycleParseXML(xmlXPathContextPtr ctxt, - const char *xpath, - int *val, - int defaultVal, - virLifecycleFromStringFunc convFunc) +static int virDomainEventActionParseXML(xmlXPathContextPtr ctxt, + const char *name, + const char *xpath, + int *val, + int defaultVal, + virEventActionFromStringFunc convFunc) { char *tmp = virXPathString(xpath, ctxt); if (tmp == NULL) { @@ -7531,7 +7532,7 @@ static int virDomainLifecycleParseXML(xmlXPathContextPtr ctxt, *val = convFunc(tmp); if (*val < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown lifecycle action %s"), tmp); + _("unknown %s action: %s"), name, tmp); VIR_FREE(tmp); return -1; } @@ -8941,20 +8942,25 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, VIR_FREE(nodes); } - if (virDomainLifecycleParseXML(ctxt, "string(./on_reboot[1])", - &def->onReboot, VIR_DOMAIN_LIFECYCLE_RESTART, - virDomainLifecycleTypeFromString) < 0) + if (virDomainEventActionParseXML(ctxt, "on_reboot", + "string(./on_reboot[1])", + &def->onReboot, + VIR_DOMAIN_LIFECYCLE_RESTART, + virDomainLifecycleTypeFromString) < 0) goto error; - if (virDomainLifecycleParseXML(ctxt, "string(./on_poweroff[1])", - &def->onPoweroff, VIR_DOMAIN_LIFECYCLE_DESTROY, - virDomainLifecycleTypeFromString) < 0) + if (virDomainEventActionParseXML(ctxt, "on_poweroff", + "string(./on_poweroff[1])", + &def->onPoweroff, + VIR_DOMAIN_LIFECYCLE_DESTROY, + virDomainLifecycleTypeFromString) < 0) goto error; - if (virDomainLifecycleParseXML(ctxt, "string(./on_crash[1])", - &def->onCrash, - VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY, - virDomainLifecycleCrashTypeFromString) < 0) + if (virDomainEventActionParseXML(ctxt, "on_crash", + "string(./on_crash[1])", + &def->onCrash, + VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY, + virDomainLifecycleCrashTypeFromString) < 0) goto error; if (virDomainPMStateParseXML(ctxt, @@ -11452,15 +11458,15 @@ virDomainEmulatorPinDel(virDomainDefPtr def) } static int -virDomainLifecycleDefFormat(virBufferPtr buf, - int type, - const char *name, - virLifecycleToStringFunc convFunc) +virDomainEventActionDefFormat(virBufferPtr buf, + int type, + const char *name, + virEventActionToStringFunc convFunc) { const char *typeStr = convFunc(type); if (!typeStr) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected lifecycle type %d"), type); + _("unexpected %s action: %d"), name, type); return -1; } @@ -13674,17 +13680,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, " </clock>\n"); } - if (virDomainLifecycleDefFormat(buf, def->onPoweroff, - "on_poweroff", - virDomainLifecycleTypeToString) < 0) + if (virDomainEventActionDefFormat(buf, def->onPoweroff, + "on_poweroff", + virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(buf, def->onReboot, - "on_reboot", - virDomainLifecycleTypeToString) < 0) + if (virDomainEventActionDefFormat(buf, def->onReboot, + "on_reboot", + virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(buf, def->onCrash, - "on_crash", - virDomainLifecycleCrashTypeToString) < 0) + if (virDomainEventActionDefFormat(buf, def->onCrash, + "on_crash", + virDomainLifecycleCrashTypeToString) < 0) goto cleanup; if (def->pm.s3 || def->pm.s4) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..0ae6e48 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2144,8 +2144,8 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model); -typedef const char* (*virLifecycleToStringFunc)(int type); -typedef int (*virLifecycleFromStringFunc)(const char *type); +typedef const char* (*virEventActionToStringFunc)(int type); +typedef int (*virEventActionFromStringFunc)(const char *type); VIR_ENUM_DECL(virDomainTaint) -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:31PM +0200, Jiri Denemark wrote:
While current on_{poweroff,reboot,crash} action configuration is about configuring life cycle actions, they can all be considered events and actions that need to be done on a particular event. Let's generalize the code this way so that it can be reused later for non-lifecycle events. --- docs/formatdomain.html.in | 15 ++++++---- docs/schemas/domaincommon.rng | 8 +++--- src/conf/domain_conf.c | 66 +++++++++++++++++++++++-------------------- src/conf/domain_conf.h | 4 +-- 4 files changed, 51 insertions(+), 42 deletions(-)
ACK By 'generalize' it looks like you just meant 'rename', since I don't see any real functional change there Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Using this new element, one can configure an action that should be performed when resource locks are lost. --- docs/formatdomain.html.in | 22 ++++++++++++++++++++++ docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 14 ++++++++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 79 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 57444fb..1001055 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -919,6 +919,7 @@ <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>restart</on_crash> + <on_lockfailure>poweroff</on_lockfailure> ...</pre> <p> @@ -974,6 +975,27 @@ domain will be restarted with the same configuration</dd> </dl> + <p> + The <code>on_lockfailure</code> element (<span class="since">since + 0.10.3</span>) may be used to configure what action should be + taken when a lock manager loses resource locks. The following + actions are recognized by libvirt, although not all of them need + to be supported by individual lock managers. When no action is + specified, each lock manager will take its default action. + </p> + <dl> + <dt><code>poweroff</code></dt> + <dd>The domain will be forcefully powered off.</dd> + <dt><code>restart</code></dt> + <dd>The domain will be powered off and started up again to + reacquire its locks.</dd> + <dt><code>pause</code></dt> + <dd>The domain will be paused so that it can be manually resumed + when lock issues are solved.</dd> + <dt><code>ignore</code></dt> + <dd>Keep the domain running as if nothing happened.</dd> + </dl> + <h3><a name="elementsPowerManagement">Power Management</a></h3> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ab8d4a1..2df2efa 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2246,6 +2246,11 @@ <ref name="crashOptions"/> </element> </optional> + <optional> + <element name="on_lockfailure"> + <ref name="lockfailureOptions"/> + </element> + </optional> </interleave> </define> <!-- @@ -2288,6 +2293,23 @@ </choice> </define> <!-- + Options when resource locks are lost: + poweroff: power off the domain + restart: power off the domain and start it up again to reacquire the + locks + pause: pause the execution of the domain so that it can be manually + resumed when lock issues are solved + ignore: keep the domain running + --> + <define name="lockfailureOptions"> + <choice> + <value>poweroff</value> + <value>restart</value> + <value>pause</value> + <value>ignore</value> + </choice> + </define> + <!-- Control ACPI sleep states (dis)allowed for the domain For each of the states the following rules apply: on: the state will be forcefully enabled diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40abf39..120d82f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -134,6 +134,13 @@ VIR_ENUM_IMPL(virDomainLifecycleCrash, VIR_DOMAIN_LIFECYCLE_CRASH_LAST, "coredump-destroy", "coredump-restart") +VIR_ENUM_IMPL(virDomainLockFailure, VIR_DOMAIN_LOCK_FAILURE_LAST, + "default", + "poweroff", + "restart", + "pause", + "ignore") + VIR_ENUM_IMPL(virDomainPMState, VIR_DOMAIN_PM_STATE_LAST, "default", "yes", @@ -8963,6 +8970,13 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, virDomainLifecycleCrashTypeFromString) < 0) goto error; + if (virDomainEventActionParseXML(ctxt, "on_lockfailure", + "string(./on_lockfailure[1])", + &def->onLockFailure, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, + virDomainLockFailureTypeFromString) < 0) + goto error; + if (virDomainPMStateParseXML(ctxt, "string(./pm/suspend-to-mem/@enabled)", &def->pm.s3) < 0) @@ -13692,6 +13706,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, "on_crash", virDomainLifecycleCrashTypeToString) < 0) goto cleanup; + if (def->onLockFailure != VIR_DOMAIN_LOCK_FAILURE_DEFAULT && + virDomainEventActionDefFormat(buf, def->onLockFailure, + "on_lockfailure", + virDomainLockFailureTypeToString) < 0) + goto cleanup; if (def->pm.s3 || def->pm.s4) { virBufferAddLit(buf, " <pm>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0ae6e48..c41e0cf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1413,6 +1413,18 @@ enum virDomainLifecycleCrashAction { VIR_DOMAIN_LIFECYCLE_CRASH_LAST }; +typedef enum { + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, + VIR_DOMAIN_LOCK_FAILURE_POWEROFF, + VIR_DOMAIN_LOCK_FAILURE_RESTART, + VIR_DOMAIN_LOCK_FAILURE_PAUSE, + VIR_DOMAIN_LOCK_FAILURE_IGNORE, + + VIR_DOMAIN_LOCK_FAILURE_LAST +} virDomainLockFailureAction; + +VIR_ENUM_DECL(virDomainLockFailure) + enum virDomainPMState { VIR_DOMAIN_PM_STATE_DEFAULT = 0, VIR_DOMAIN_PM_STATE_ENABLED, @@ -1681,6 +1693,8 @@ struct _virDomainDef { int onPoweroff; int onCrash; + int onLockFailure; /* enum virDomainLockFailureAction */ + struct { /* These options are actually type of enum virDomainPMState */ int s3; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dab607a..9e04d2b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -421,6 +421,8 @@ virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; virDomainLiveConfigHelperMethod; virDomainLoadAllConfigs; +virDomainLockFailureTypeFromString; +virDomainLockFailureTypeToString; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemDumpTypeFromString; -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:32PM +0200, Jiri Denemark wrote:
Using this new element, one can configure an action that should be performed when resource locks are lost. --- docs/formatdomain.html.in | 22 ++++++++++++++++++++++ docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 14 ++++++++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 79 insertions(+)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 18 +++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ca2f694..fe8de36 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -47,6 +47,8 @@ # define QEMUD_CPUMASK_LEN CPU_SETSIZE +# define QEMU_URI_SCHEME "qemu" + typedef struct _qemuDriverCloseDef qemuDriverCloseDef; typedef qemuDriverCloseDef *qemuDriverCloseDefPtr; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 95a30e6..e1be849 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -295,8 +295,8 @@ qemuAutostartDomains(struct qemud_driver *driver) * network */ virConnectPtr conn = virConnectOpen(driver->privileged ? - "qemu:///system" : - "qemu:///session"); + QEMU_URI_SCHEME ":///system" : + QEMU_URI_SCHEME ":///session"); /* Ignoring NULL conn which is mostly harmless here */ struct qemuAutostartData data = { driver, conn }; @@ -859,8 +859,8 @@ qemudStartup(int privileged) { virHashForEach(qemu_driver->domains.objs, qemuDomainNetsRestart, NULL); conn = virConnectOpen(qemu_driver->privileged ? - "qemu:///system" : - "qemu:///session"); + QEMU_URI_SCHEME ":///system" : + QEMU_URI_SCHEME ":///session"); qemuProcessReconnectAll(conn, qemu_driver); @@ -1054,13 +1054,13 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, return VIR_DRV_OPEN_DECLINED; if (!(conn->uri = virURIParse(qemu_driver->privileged ? - "qemu:///system" : - "qemu:///session"))) + QEMU_URI_SCHEME ":///system" : + QEMU_URI_SCHEME ":///session"))) return VIR_DRV_OPEN_ERROR; } else { /* If URI isn't 'qemu' its definitely not for us */ if (conn->uri->scheme == NULL || - STRNEQ(conn->uri->scheme, "qemu")) + STRNEQ(conn->uri->scheme, QEMU_URI_SCHEME)) return VIR_DRV_OPEN_DECLINED; /* Allow remote driver to deal with URIs with hostname server */ @@ -1077,8 +1077,8 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("no QEMU URI path given, try %s"), qemu_driver->privileged - ? "qemu:///system" - : "qemu:///session"); + ? QEMU_URI_SCHEME ":///system" + : QEMU_URI_SCHEME ":///session"); return VIR_DRV_OPEN_ERROR; } -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:33PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 18 +++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ca2f694..fe8de36 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -47,6 +47,8 @@
# define QEMUD_CPUMASK_LEN CPU_SETSIZE
+# define QEMU_URI_SCHEME "qemu" + typedef struct _qemuDriverCloseDef qemuDriverCloseDef; typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 95a30e6..e1be849 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -295,8 +295,8 @@ qemuAutostartDomains(struct qemud_driver *driver) * network */ virConnectPtr conn = virConnectOpen(driver->privileged ? - "qemu:///system" : - "qemu:///session"); + QEMU_URI_SCHEME ":///system" : + QEMU_URI_SCHEME ":///session"); /* Ignoring NULL conn which is mostly harmless here */ struct qemuAutostartData data = { driver, conn };
@@ -859,8 +859,8 @@ qemudStartup(int privileged) { virHashForEach(qemu_driver->domains.objs, qemuDomainNetsRestart, NULL);
conn = virConnectOpen(qemu_driver->privileged ? - "qemu:///system" : - "qemu:///session"); + QEMU_URI_SCHEME ":///system" : + QEMU_URI_SCHEME ":///session");
qemuProcessReconnectAll(conn, qemu_driver);
@@ -1054,13 +1054,13 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, return VIR_DRV_OPEN_DECLINED;
if (!(conn->uri = virURIParse(qemu_driver->privileged ? - "qemu:///system" : - "qemu:///session"))) + QEMU_URI_SCHEME ":///system" : + QEMU_URI_SCHEME ":///session"))) return VIR_DRV_OPEN_ERROR; } else { /* If URI isn't 'qemu' its definitely not for us */ if (conn->uri->scheme == NULL || - STRNEQ(conn->uri->scheme, "qemu")) + STRNEQ(conn->uri->scheme, QEMU_URI_SCHEME)) return VIR_DRV_OPEN_DECLINED;
/* Allow remote driver to deal with URIs with hostname server */ @@ -1077,8 +1077,8 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("no QEMU URI path given, try %s"), qemu_driver->privileged - ? "qemu:///system" - : "qemu:///session"); + ? QEMU_URI_SCHEME ":///system" + : QEMU_URI_SCHEME ":///session"); return VIR_DRV_OPEN_ERROR; }
I'm not really convinced that this is a net win - in fact I think it makes the code a little less readable in general. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- src/locking/lock_driver.h | 2 ++ src/locking/lock_manager.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 83ef323..c33aea7 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -66,6 +66,7 @@ typedef enum { enum { VIR_LOCK_MANAGER_PARAM_TYPE_STRING, + VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, VIR_LOCK_MANAGER_PARAM_TYPE_INT, VIR_LOCK_MANAGER_PARAM_TYPE_LONG, VIR_LOCK_MANAGER_PARAM_TYPE_UINT, @@ -84,6 +85,7 @@ struct _virLockManagerParam { unsigned long long ul; double d; char *str; + const char *cstr; unsigned char uuid[16]; } value; }; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index f5e967f..23126c9 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -99,6 +99,9 @@ static void virLockManagerLogParams(size_t nparams, case VIR_LOCK_MANAGER_PARAM_TYPE_STRING: VIR_DEBUG(" key=%s type=string value=%s", params[i].key, params[i].value.str); break; + case VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING: + VIR_DEBUG(" key=%s type=cstring value=%s", params[i].key, params[i].value.cstr); + break; case VIR_LOCK_MANAGER_PARAM_TYPE_UUID: virUUIDFormat(params[i].value.uuid, uuidstr); VIR_DEBUG(" key=%s type=uuid value=%s", params[i].key, uuidstr); -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:34PM +0200, Jiri Denemark wrote:
--- src/locking/lock_driver.h | 2 ++ src/locking/lock_manager.c | 3 +++ 2 files changed, 5 insertions(+)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 83ef323..c33aea7 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -66,6 +66,7 @@ typedef enum {
enum { VIR_LOCK_MANAGER_PARAM_TYPE_STRING, + VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, VIR_LOCK_MANAGER_PARAM_TYPE_INT, VIR_LOCK_MANAGER_PARAM_TYPE_LONG, VIR_LOCK_MANAGER_PARAM_TYPE_UINT, @@ -84,6 +85,7 @@ struct _virLockManagerParam { unsigned long long ul; double d; char *str; + const char *cstr; unsigned char uuid[16]; } value; }; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index f5e967f..23126c9 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -99,6 +99,9 @@ static void virLockManagerLogParams(size_t nparams, case VIR_LOCK_MANAGER_PARAM_TYPE_STRING: VIR_DEBUG(" key=%s type=string value=%s", params[i].key, params[i].value.str); break; + case VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING: + VIR_DEBUG(" key=%s type=cstring value=%s", params[i].key, params[i].value.cstr); + break; case VIR_LOCK_MANAGER_PARAM_TYPE_UUID: virUUIDFormat(params[i].value.uuid, uuidstr); VIR_DEBUG(" key=%s type=uuid value=%s", params[i].key, uuidstr);
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This is required in case a lock manager needs to contact libvirtd in case of an unexpected event. --- docs/internals/locking.html.in | 8 ++++++++ src/locking/domain_lock.c | 25 +++++++++++++++++-------- src/locking/domain_lock.h | 4 ++++ src/locking/lock_driver.h | 1 + src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_hotplug.c | 15 ++++++++++----- src/qemu/qemu_process.c | 4 +++- 7 files changed, 45 insertions(+), 15 deletions(-) diff --git a/docs/internals/locking.html.in b/docs/internals/locking.html.in index e84321b..b43fe7a 100644 --- a/docs/internals/locking.html.in +++ b/docs/internals/locking.html.in @@ -183,6 +183,10 @@ .key = "pid", .value = { .i = dom->pid }, }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, + .key = "driver", + .value = { .cstr = driver->name }, + }, }; mgr = virLockManagerNew(lockPlugin, VIR_LOCK_MANAGER_TYPE_DOMAIN, @@ -225,6 +229,10 @@ .key = "pid", .value = { .i = dom->pid }, }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, + .key = "driver", + .value = { .cstr = driver->name }, + }, }; mgr = virLockManagerNew(lockPlugin, VIR_LOCK_MANAGER_TYPE_DOMAIN, diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 3417361..999b9a0 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -97,6 +97,7 @@ static int virDomainLockManagerAddDisk(virLockManagerPtr lock, } static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, bool withResources) { @@ -118,6 +119,10 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, .key = "pid", .value = { .i = dom->pid }, }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, + .key = "driver", + .value = { .cstr = driver }, + }, }; VIR_DEBUG("plugin=%p dom=%p withResources=%d", plugin, dom, withResources); @@ -152,6 +157,7 @@ error: int virDomainLockProcessStart(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, bool paused, int *fd) @@ -163,7 +169,7 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", plugin, dom, paused, fd); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, driver, dom, true))) return -1; if (paused) @@ -186,7 +192,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true))) return -1; ret = virLockManagerRelease(lock, state, 0); @@ -196,6 +202,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, } int virDomainLockProcessResume(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, const char *state) { @@ -205,7 +212,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%s", plugin, dom, NULLSTR(state)); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, driver, dom, true))) return -1; ret = virLockManagerAcquire(lock, state, 0, NULL); @@ -224,7 +231,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true))) return -1; ret = virLockManagerInquire(lock, state, 0); @@ -235,6 +242,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, virDomainDiskDefPtr disk) { @@ -244,7 +252,7 @@ int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p disk=%p", plugin, dom, disk); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, driver, dom, false))) return -1; if (virDomainLockManagerAddDisk(lock, disk) < 0) @@ -271,7 +279,7 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p disk=%p", plugin, dom, disk); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) return -1; if (virDomainLockManagerAddDisk(lock, disk) < 0) @@ -290,6 +298,7 @@ cleanup: int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, virDomainLeaseDefPtr lease) { @@ -299,7 +308,7 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, driver, dom, false))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -326,7 +335,7 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index 3bedeb1..42f80a2 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -27,6 +27,7 @@ # include "lock_manager.h" int virDomainLockProcessStart(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, bool paused, int *fd); @@ -34,6 +35,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, virDomainObjPtr dom, char **state); int virDomainLockProcessResume(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, const char *state); int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, @@ -41,6 +43,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, char **state); int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, virDomainDiskDefPtr disk); int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, @@ -48,6 +51,7 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, virDomainDiskDefPtr disk); int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, + const char *driver, virDomainObjPtr dom, virDomainLeaseDefPtr lease); int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index c33aea7..988a71a 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -155,6 +155,7 @@ typedef int (*virLockDriverDeinit)(void); * - uuid: the domain uuid (uuid) * - name: the domain name (string) * - pid: process ID to own/owning the lock (unsigned int) + * - driver: the name of the libvirtd driver the domain belongs to (string) * * Returns 0 if successful initialized a new context, -1 on error */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1be849..9b0e6b3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10692,7 +10692,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = disk->driverType; disk->driverType = (char *) "raw"; /* Don't want to probe backing files */ - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, disk) < 0) goto cleanup; if (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a738b19..87a3cc4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -88,7 +88,8 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, return -1; } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -217,7 +218,8 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn, } } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -449,7 +451,8 @@ int qemuDomainAttachSCSIDisk(virConnectPtr conn, } } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -572,7 +575,8 @@ int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn, } } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -2389,7 +2393,8 @@ int qemuDomainAttachLease(struct qemud_driver *driver, if (virDomainLeaseInsertPreAlloc(vm->def) < 0) return -1; - if (virDomainLockLeaseAttach(driver->lockManager, vm, lease) < 0) { + if (virDomainLockLeaseAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, lease) < 0) { virDomainLeaseInsertPreAlloced(vm->def, NULL); return -1; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 31909b7..de35714 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2597,6 +2597,7 @@ static int qemuProcessHook(void *data) if (virSecurityManagerSetSocketLabel(h->driver->securityManager, h->vm->def) < 0) goto cleanup; if (virDomainLockProcessStart(h->driver->lockManager, + QEMU_URI_SCHEME, h->vm, /* QEMU is always pased initially */ true, @@ -2666,7 +2667,8 @@ qemuProcessStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); - if (virDomainLockProcessResume(driver->lockManager, vm, priv->lockState) < 0) { + if (virDomainLockProcessResume(driver->lockManager, QEMU_URI_SCHEME, + vm, priv->lockState) < 0) { /* Don't free priv->lockState on error, because we need * to make sure we have state still present if the user * tries to resume again -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:35PM +0200, Jiri Denemark wrote:
This is required in case a lock manager needs to contact libvirtd in case of an unexpected event. --- docs/internals/locking.html.in | 8 ++++++++ src/locking/domain_lock.c | 25 +++++++++++++++++-------- src/locking/domain_lock.h | 4 ++++ src/locking/lock_driver.h | 1 + src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_hotplug.c | 15 ++++++++++----- src/qemu/qemu_process.c | 4 +++- 7 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1be849..9b0e6b3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10692,7 +10692,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = disk->driverType; disk->driverType = (char *) "raw"; /* Don't want to probe backing files */
- if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, QEMU_URI_SCHEME, + vm, disk) < 0) goto cleanup;
My inclination is that we ought to be passing 'qemu:///system' or 'qemu:///session' instead of just 'qemu'. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- src/Makefile.am | 2 +- src/locking/domain_lock.c | 11 +++++++---- src/locking/lock_driver.h | 5 ++++- src/locking/lock_driver_nop.c | 1 + src/locking/lock_driver_sanlock.c | 1 + src/locking/lock_manager.c | 7 ++++--- src/locking/lock_manager.h | 1 + 7 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index ae3d491..bddff7e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1492,7 +1492,7 @@ lockdriverdir = $(libdir)/libvirt/lock-driver lockdriver_LTLIBRARIES = sanlock.la sanlock_la_SOURCES = $(LOCK_DRIVER_SANLOCK_SOURCES) -sanlock_la_CFLAGS = $(AM_CFLAGS) +sanlock_la_CFLAGS = -I$(top_srcdir)/src/conf $(AM_CFLAGS) sanlock_la_LDFLAGS = -module -avoid-version sanlock_la_LIBADD = -lsanlock_client \ ../gnulib/lib/libgnu.la diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 999b9a0..6ef539a 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -175,7 +175,8 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, if (paused) flags |= VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY; - ret = virLockManagerAcquire(lock, NULL, flags, fd); + ret = virLockManagerAcquire(lock, NULL, flags, + dom->def->onLockFailure, fd); virLockManagerFree(lock); @@ -215,7 +216,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, if (!(lock = virDomainLockManagerNew(plugin, driver, dom, true))) return -1; - ret = virLockManagerAcquire(lock, state, 0, NULL); + ret = virLockManagerAcquire(lock, state, 0, dom->def->onLockFailure, NULL); virLockManagerFree(lock); return ret; @@ -258,7 +259,8 @@ int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, if (virDomainLockManagerAddDisk(lock, disk) < 0) goto cleanup; - if (virLockManagerAcquire(lock, NULL, 0, NULL) < 0) + if (virLockManagerAcquire(lock, NULL, 0, + dom->def->onLockFailure, NULL) < 0) goto cleanup; ret = 0; @@ -314,7 +316,8 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, if (virDomainLockManagerAddLease(lock, lease) < 0) goto cleanup; - if (virLockManagerAcquire(lock, NULL, 0, NULL) < 0) + if (virLockManagerAcquire(lock, NULL, 0, + dom->def->onLockFailure, NULL) < 0) goto cleanup; ret = 0; diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 988a71a..95e8311 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -23,6 +23,7 @@ # define __VIR_PLUGINS_LOCK_DRIVER_H__ # include "internal.h" +# include "domain_conf.h" typedef struct _virLockManager virLockManager; typedef virLockManager *virLockManagerPtr; @@ -218,12 +219,13 @@ typedef int (*virLockDriverAddResource)(virLockManagerPtr man, * @manager: the lock manager context * @state: the current lock state * @flags: optional flags, currently unused + * @action: action to take when lock is lost * @fd: optional return the leaked FD * * Start managing resources for the object. This * must be called from the PID that represents the * object to be managed. If the lock is lost at any - * time, the PID will be killed off by the lock manager. + * time, the specified action will be taken. * The optional state contains information about the * locks previously held for the object. * @@ -237,6 +239,7 @@ typedef int (*virLockDriverAddResource)(virLockManagerPtr man, typedef int (*virLockDriverAcquire)(virLockManagerPtr man, const char *state, unsigned int flags, + virDomainLockFailureAction action, int *fd); /** diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c index 7f6cdca..c9ab806 100644 --- a/src/locking/lock_driver_nop.c +++ b/src/locking/lock_driver_nop.c @@ -69,6 +69,7 @@ static int virLockManagerNopAddResource(virLockManagerPtr lock ATTRIBUTE_UNUSED, static int virLockManagerNopAcquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, const char *state ATTRIBUTE_UNUSED, unsigned int flags_unused ATTRIBUTE_UNUSED, + virDomainLockFailureAction action ATTRIBUTE_UNUSED, int *fd ATTRIBUTE_UNUSED) { return 0; diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 113fd47..8c0ac8c 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -686,6 +686,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, static int virLockManagerSanlockAcquire(virLockManagerPtr lock, const char *state, unsigned int flags, + virDomainLockFailureAction action ATTRIBUTE_UNUSED, int *fd) { virLockManagerSanlockPrivatePtr priv = lock->privateData; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 23126c9..423997b 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -347,17 +347,18 @@ int virLockManagerAddResource(virLockManagerPtr lock, int virLockManagerAcquire(virLockManagerPtr lock, const char *state, unsigned int flags, + virDomainLockFailureAction action, int *fd) { - VIR_DEBUG("lock=%p state='%s' flags=%x fd=%p", - lock, NULLSTR(state), flags, fd); + VIR_DEBUG("lock=%p state='%s' flags=%x action=%d fd=%p", + lock, NULLSTR(state), flags, action, fd); CHECK_MANAGER(drvAcquire, -1); if (fd) *fd = -1; - return lock->driver->drvAcquire(lock, state, flags, fd); + return lock->driver->drvAcquire(lock, state, flags, action, fd); } diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h index d955ee0..4fee12d 100644 --- a/src/locking/lock_manager.h +++ b/src/locking/lock_manager.h @@ -56,6 +56,7 @@ int virLockManagerAddResource(virLockManagerPtr manager, int virLockManagerAcquire(virLockManagerPtr manager, const char *state, unsigned int flags, + virDomainLockFailureAction action, int *fd); int virLockManagerRelease(virLockManagerPtr manager, char **state, -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:36PM +0200, Jiri Denemark wrote:
--- src/Makefile.am | 2 +- src/locking/domain_lock.c | 11 +++++++---- src/locking/lock_driver.h | 5 ++++- src/locking/lock_driver_nop.c | 1 + src/locking/lock_driver_sanlock.c | 1 + src/locking/lock_manager.c | 7 ++++--- src/locking/lock_manager.h | 1 + 7 files changed, 19 insertions(+), 9 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

While the changes to sanlock driver should be stable, the actual implementation of sanlock_helper is supposed to be replaced in the future. However, before we can implement a better sanlock_helper, we need an administrative interface to libvirtd so that the helper can just pass a "leases lost" event to the particular libvirt driver and everything else will be taken care of internally. This approach will also allow libvirt to pass such event to applications and use appropriate reasons when changing domain states. The temporary implementation handles all actions directly by calling appropriate libvirt APIs (which among other things means that it needs to know the credentials required to connect to libvirtd). --- libvirt.spec.in | 1 + src/Makefile.am | 11 ++ src/locking/lock_driver_sanlock.c | 115 +++++++++++++++++--- src/locking/sanlock_helper.c | 214 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 328 insertions(+), 13 deletions(-) create mode 100644 src/locking/sanlock_helper.c diff --git a/libvirt.spec.in b/libvirt.spec.in index 11e3199..2d834c6 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1788,6 +1788,7 @@ rm -f $RPM_BUILD_ROOT%{_sysconfdir}/sysctl.d/libvirtd %dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/sanlock %{_sbindir}/virt-sanlock-cleanup %{_mandir}/man8/virt-sanlock-cleanup.8* +%attr(0755, root, root) %{_libexecdir}/libvirt_sanlock_helper %endif %files client -f %{name}.lang diff --git a/src/Makefile.am b/src/Makefile.am index bddff7e..6fe2eb1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -142,6 +142,8 @@ DRIVER_SOURCES = \ LOCK_DRIVER_SANLOCK_SOURCES = \ locking/lock_driver_sanlock.c +LOCK_DRIVER_SANLOCK_HELPER_SOURCES = \ + locking/sanlock_helper.c NETDEV_CONF_SOURCES = \ conf/netdev_bandwidth_conf.h conf/netdev_bandwidth_conf.c \ @@ -1639,6 +1641,15 @@ endif EXTRA_DIST += $(STORAGE_HELPER_DISK_SOURCES) +if HAVE_SANLOCK +libexec_PROGRAMS += libvirt_sanlock_helper + +libvirt_sanlock_helper_SOURCES = $(LOCK_DRIVER_SANLOCK_HELPER_SOURCES) +libvirt_sanlock_helper_CFLAGS = -I$(top_srcdir)/src/conf $(AM_CFLAGS) +libvirt_sanlock_helper_LDFLAGS = $(WARN_LDFLAGS) $(AM_LDFLAGS) +libvirt_sanlock_helper_LDADD = libvirt.la +endif + if WITH_LXC if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_lxc diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 8c0ac8c..804dc90 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -50,6 +50,7 @@ #define VIR_FROM_THIS VIR_FROM_LOCKING #define VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE "__LIBVIRT__DISKS__" +#define VIR_LOCK_MANAGER_SANLOCK_KILLPATH LIBEXECDIR "/libvirt_sanlock_helper" /* * temporary fix for the case where the sanlock devel package is @@ -75,8 +76,9 @@ struct _virLockManagerSanlockDriver { static virLockManagerSanlockDriver *driver = NULL; struct _virLockManagerSanlockPrivate { + const char *driver; char vm_name[SANLK_NAME_LEN]; - char vm_uuid[VIR_UUID_BUFLEN]; + unsigned char vm_uuid[VIR_UUID_BUFLEN]; unsigned int vm_id; unsigned int vm_pid; unsigned int flags; @@ -383,6 +385,8 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, priv->vm_pid = param->value.ui; } else if (STREQ(param->key, "id")) { priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "driver")) { + priv->driver = param->value.cstr; } } @@ -683,10 +687,86 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, return 0; } +static int +virLockManagerSanlockRegisterKillscript(int sock, + const char *vmdriver, + const char *uuidstr, + virDomainLockFailureAction action) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *path; + char *args = NULL; + int ret = -1; + int rv; + + if (action > VIR_DOMAIN_LOCK_FAILURE_IGNORE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Failure action %s is not supported by sanlock"), + virDomainLockFailureTypeToString(action)); + goto cleanup; + } + + virBufferEscape(&buf, '\\', "\\ ", "%s", vmdriver); + virBufferAddLit(&buf, " "); + virBufferEscape(&buf, '\\', "\\ ", "%s", uuidstr); + virBufferAddLit(&buf, " "); + virBufferEscape(&buf, '\\', "\\ ", "%s", + virDomainLockFailureTypeToString(action)); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto cleanup; + } + + /* Unfortunately, sanlock_killpath() does not use const for either + * path or args even though it will just copy them into its own + * buffers. + */ + path = (char *) VIR_LOCK_MANAGER_SANLOCK_KILLPATH; + args = virBufferContentAndReset(&buf); + + VIR_DEBUG("Register sanlock killpath: %s %s", path, args); + + /* sanlock_killpath() would just crop the strings */ + if (strlen(path) >= SANLK_HELPER_PATH_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Sanlock helper path is longer than %d: '%s'"), + SANLK_HELPER_PATH_LEN - 1, path); + goto cleanup; + } + if (strlen(args) >= SANLK_HELPER_ARGS_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Sanlock helper arguments are longer than %d:" + " '%s'"), + SANLK_HELPER_ARGS_LEN - 1, args); + goto cleanup; + } + + if ((rv = sanlock_killpath(sock, 0, path, args)) < 0) { + if (rv <= -200) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register lock failure action:" + " error %d"), rv); + } else { + virReportSystemError(-rv, "%s", + _("Failed to register lock failure" + " action")); + } + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(args); + return ret; +} + static int virLockManagerSanlockAcquire(virLockManagerPtr lock, const char *state, unsigned int flags, - virDomainLockFailureAction action ATTRIBUTE_UNUSED, + virDomainLockFailureAction action, int *fd) { virLockManagerSanlockPrivatePtr priv = lock->privateData; @@ -741,23 +821,32 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, res_count = priv->res_count; } - VIR_DEBUG("Register sanlock %d", flags); /* We only initialize 'sock' if we are in the real * child process and we need it to be inherited * * If sock==-1, then sanlock auto-open/closes a * temporary sock */ - if (priv->vm_pid == getpid() && - (sock = sanlock_register()) < 0) { - if (sock <= -200) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to open socket to sanlock daemon: error %d"), - sock); - else - virReportSystemError(-sock, "%s", - _("Failed to open socket to sanlock daemon")); - goto error; + if (priv->vm_pid == getpid()) { + VIR_DEBUG("Register sanlock %d", flags); + if ((sock = sanlock_register()) < 0) { + if (sock <= -200) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open socket to sanlock daemon: error %d"), + sock); + else + virReportSystemError(-sock, "%s", + _("Failed to open socket to sanlock daemon")); + goto error; + } + + if (action != VIR_DOMAIN_LOCK_FAILURE_DEFAULT) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(priv->vm_uuid, uuidstr); + if (virLockManagerSanlockRegisterKillscript(sock, priv->driver, + uuidstr, action) < 0) + goto error; + } } if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) { diff --git a/src/locking/sanlock_helper.c b/src/locking/sanlock_helper.c new file mode 100644 index 0000000..2d2f0f9 --- /dev/null +++ b/src/locking/sanlock_helper.c @@ -0,0 +1,214 @@ +#include <config.h> +#include <stdio.h> +#include <stdlib.h> + +#include "configmake.h" +#include "internal.h" +#include "conf.h" +#include "memory.h" +#include "domain_conf.h" + +static char *login = NULL; +static char *password = NULL; + +static int +getArgs(int argc, + char **argv, + const char **driver, + const char **uuid, + virDomainLockFailureAction *action) +{ + int act; + + if (argc != 4) { + fprintf(stderr, "%s driver uuid action\n", argv[0]); + return -1; + } + + *driver = argv[1]; + *uuid = argv[2]; + + act = virDomainLockFailureTypeFromString(argv[3]); + if (act < 0) { + fprintf(stderr, "invalid failure action: '%s'\n", argv[3]); + return -1; + } + *action = act; + + return 0; +} + + +static int +getConfig(virConfPtr conf, const char *name, char **value) +{ + virConfValuePtr p; + + if ((p = virConfGetValue(conf, name)) && + (p->type != VIR_CONF_STRING || + !(*value = strdup(p->str)))) + return -1; + + return 0; +} + +static int +getCredentials(const char *driver) +{ + virConfPtr conf = NULL; + char *path; + int ret = -1; + + if (virAsprintf(&path, "%s/libvirt/%s-sanlock.conf", + SYSCONFDIR, driver) < 0) + return -1; + + if (!(conf = virConfReadFile(path, 0))) { + ret = 0; + goto cleanup; + } + + if (getConfig(conf, "helper_login", &login) < 0 || + getConfig(conf, "helper_password", &password) < 0 || + (login && !password) || (!login && password)) { + fprintf(stderr, "incorrect login or password\n"); + VIR_FREE(login); + VIR_FREE(password); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(path); + virConfFree(conf); + return ret; +} + + +static int +authCallback(virConnectCredentialPtr cred, + unsigned int ncred, + void *cbdata ATTRIBUTE_UNUSED) +{ + int i; + + for (i = 0 ; i < ncred ; i++) { + switch (cred[i].type) { + case VIR_CRED_AUTHNAME: + case VIR_CRED_ECHOPROMPT: + if (!login) + return -1; + cred[i].result = strdup(login); + break; + + case VIR_CRED_PASSPHRASE: + case VIR_CRED_NOECHOPROMPT: + if (!password) + return -1; + cred[i].result = strdup(password); + break; + + default: + return -1; + } + + if (!cred[i].result) + return -1; + cred[i].resultlen = strlen(cred[i].result); + } + + return 0; +} + + +int +main(int argc, char **argv) +{ + const char *driver; + const char *uuid; + virDomainLockFailureAction action; + char *uri = NULL; + char *xml = NULL; + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + int ret = EXIT_FAILURE; + + int authTypes[] = { + VIR_CRED_AUTHNAME, + VIR_CRED_ECHOPROMPT, + VIR_CRED_PASSPHRASE, + VIR_CRED_NOECHOPROMPT, + }; + virConnectAuth auth = { + .credtype = authTypes, + .ncredtype = ARRAY_CARDINALITY(authTypes), + .cb = authCallback, + }; + + if (getArgs(argc, argv, &driver, &uuid, &action) < 0 || + getCredentials(driver) < 0) + goto cleanup; + + if (virAsprintf(&uri, "%s:///%s", + driver, + STREQ(driver, "qemu") ? "system" : "") < 0) + goto cleanup; + + if (!(conn = virConnectOpenAuth(uri, &auth, 0)) || + !(dom = virDomainLookupByUUIDString(conn, uuid))) + goto cleanup; + + switch (action) { + case VIR_DOMAIN_LOCK_FAILURE_POWEROFF: + if (virDomainDestroy(dom) == 0 || + virDomainIsActive(dom) == 0) + ret = EXIT_SUCCESS; + break; + + case VIR_DOMAIN_LOCK_FAILURE_RESTART: + if (virDomainIsPersistent(dom)) { + if ((virDomainDestroy(dom) == 0 || + virDomainIsActive(dom) == 0) && + virDomainCreate(dom) == 0) + ret = EXIT_SUCCESS; + } else { + xml = virDomainGetXMLDesc(dom, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INACTIVE); + + if (!xml || + (virDomainDestroy(dom) < 0 && + virDomainIsActive(dom) != 0)) + goto cleanup; + virDomainFree(dom); + if ((dom = virDomainCreateXML(conn, xml, 0))) + ret = EXIT_SUCCESS; + } + break; + + case VIR_DOMAIN_LOCK_FAILURE_PAUSE: + if (virDomainSuspend(dom) == 0) + ret = EXIT_SUCCESS; + break; + + case VIR_DOMAIN_LOCK_FAILURE_IGNORE: + ret = EXIT_SUCCESS; + break; + + default: + fprintf(stderr, "unsupported failure action: '%s'\n", + virDomainLockFailureTypeToString(action)); + break; + } + +cleanup: + if (dom) + virDomainFree(dom); + if (conn) + virConnectClose(conn); + VIR_FREE(xml); + VIR_FREE(uri); + + return ret; +} -- 1.7.12

On Thu, Sep 27, 2012 at 04:41:37PM +0200, Jiri Denemark wrote:
While the changes to sanlock driver should be stable, the actual implementation of sanlock_helper is supposed to be replaced in the future. However, before we can implement a better sanlock_helper, we need an administrative interface to libvirtd so that the helper can just pass a "leases lost" event to the particular libvirt driver and everything else will be taken care of internally. This approach will also allow libvirt to pass such event to applications and use appropriate reasons when changing domain states.
The temporary implementation handles all actions directly by calling appropriate libvirt APIs (which among other things means that it needs to know the credentials required to connect to libvirtd).
+static int +getCredentials(const char *driver) +{ + virConfPtr conf = NULL; + char *path; + int ret = -1; + + if (virAsprintf(&path, "%s/libvirt/%s-sanlock.conf", + SYSCONFDIR, driver) < 0) + return -1; + + if (!(conf = virConfReadFile(path, 0))) { + ret = 0; + goto cleanup; + } + + if (getConfig(conf, "helper_login", &login) < 0 || + getConfig(conf, "helper_password", &password) < 0 ||
Libvirt already has support built-in for loading usernames and passwords from a configuration file, so I don't think we need to duplicate that in the sanlock config file. This is just a documentation task to describe where to put the login info for sanlock to use it.
+ + if (virAsprintf(&uri, "%s:///%s", + driver, + STREQ(driver, "qemu") ? "system" : "") < 0) + goto cleanup;
This seems rather dubious to me and re-inforces my opinion that we should be passing the full URI, not the driver name. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange
-
Jiri Denemark