12:44 p.m.
These two patches implement a lock for long-running
job(save, dump and migration) and normal job which will
return in short time but maybe affect the state of virtual
machine.
With these patches, the normal job could not execute while
the long-ruuning job or other normal job is running.
In order to tracking these job, job type for normal job and
long-ruuning job(will be called as async job in code) is
defined.
The abort job is called while user want to cancel such async
job. Note that there is no cancellation function for async
job in xenlight stack. So, abort function just block the
cancellation of user.
Bamvor Jian Zhang (2):
Introduce a lock for libxl long-running api
Add lock for libxl api
src/libxl/libxl_conf.h | 58 +++++
src/libxl/libxl_driver.c | 665 ++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 662 insertions(+), 61 deletions(-)
--
1.7.12
12:44 p.m.
New subject: [libvirt] [PATCH 1/2] Introduce a lock for libxl long-running api
This patch introduce a lock for protecting the long-running
api (save, dump, migration and so on) from the other api
which may update the status of the virtual machine.
Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
---
src/libxl/libxl_conf.h | 58 ++++++
src/libxl/libxl_driver.c | 446 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 504 insertions(+)
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 56bf85c..dab1466 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -73,6 +73,62 @@ struct _libxlDriverPrivate {
char *saveDir;
};
+# define JOB_MASK(job) (1 << (job - 1))
+# define DEFAULT_JOB_MASK \
+ (JOB_MASK(LIBXL_JOB_DESTROY) | \
+ JOB_MASK(LIBXL_JOB_ABORT))
+
+/* Jobs which have to be tracked in domain state XML. */
+# define LIBXL_DOMAIN_TRACK_JOBS \
+ (JOB_MASK(LIBXL_JOB_DESTROY) | \
+ JOB_MASK(LIBXL_JOB_ASYNC))
+
+/* Only 1 job is allowed at any time
+ * A job includes *all* libxl.so api, even those just querying
+ * information, not merely actions */
+enum libxlDomainJob {
+ LIBXL_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive) conditions */
+ LIBXL_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */
+ LIBXL_JOB_MODIFY, /* May change state */
+ LIBXL_JOB_ABORT, /* Abort current async job */
+ LIBXL_JOB_MIGRATION_OP, /* Operation influencing outgoing migration */
+
+ /* The following two items must always be the last items before JOB_LAST */
+ LIBXL_JOB_ASYNC, /* Asynchronous job */
+
+ LIBXL_JOB_LAST
+};
+VIR_ENUM_DECL(libxlDomainJob)
+
+/* Async job consists of a series of jobs that may change state. Independent
+ * jobs that do not change state (and possibly others if explicitly allowed by
+ * current async job) are allowed to be run even if async job is active.
+ */
+enum libxlDomainAsyncJob {
+ LIBXL_ASYNC_JOB_NONE = 0,
+ LIBXL_ASYNC_JOB_MIGRATION_OUT,
+ LIBXL_ASYNC_JOB_MIGRATION_IN,
+ LIBXL_ASYNC_JOB_SAVE,
+ LIBXL_ASYNC_JOB_DUMP,
+
+ LIBXL_ASYNC_JOB_LAST
+};
+VIR_ENUM_DECL(libxlDomainAsyncJob)
+
+struct libxlDomainJobObj {
+ virCond cond; /* Use to coordinate jobs */
+ enum libxlDomainJob active; /* Currently running job */
+ int owner; /* Thread which set current job */
+
+ virCond asyncCond; /* Use to coordinate with async jobs */
+ enum libxlDomainAsyncJob asyncJob; /* Currently active async job */
+ int asyncOwner; /* Thread which set current async job */
+ int phase; /* Job phase (mainly for migrations) */
+ unsigned long long mask; /* Jobs allowed during async job */
+ unsigned long long start; /* When the async job started */
+ virDomainJobInfo info; /* Async job progress data */
+};
+
typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate;
typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
struct _libxlDomainObjPrivate {
@@ -81,6 +137,8 @@ struct _libxlDomainObjPrivate {
libxl_waiter *dWaiter;
int waiterFD;
int eventHdl;
+
+ struct libxlDomainJobObj job;
};
# define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 6fe284f..d01d915 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -45,6 +45,7 @@
#include "xen_xm.h"
#include "virtypedparam.h"
#include "viruri.h"
+#include "virtime.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -84,6 +85,336 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver)
virMutexUnlock(&driver->lock);
}
+/* job */
+VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST,
+ "none",
+ "destroy",
+ "modify",
+ "abort",
+ "migration operation",
+ "none", /* async job is never stored in job.active */
+);
+
+VIR_ENUM_IMPL(libxlDomainAsyncJob, LIBXL_ASYNC_JOB_LAST,
+ "none",
+ "migration out",
+ "migration in",
+ "save",
+ "dump",
+);
+
+static int
+libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
+{
+ memset(&priv->job, 0, sizeof(priv->job));
+
+ if (virCondInit(&priv->job.cond) < 0)
+ return -1;
+
+ if (virCondInit(&priv->job.asyncCond) < 0) {
+ ignore_value(virCondDestroy(&priv->job.cond));
+ return -1;
+ }
+
+ return 0;
+}
+
+static void
+libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv)
+{
+ struct libxlDomainJobObj *job = &priv->job;
+
+ job->active = LIBXL_JOB_NONE;
+ job->owner = 0;
+}
+
+static void
+libxlDomainObjResetAsyncJob(libxlDomainObjPrivatePtr priv)
+{
+ struct libxlDomainJobObj *job = &priv->job;
+
+ job->asyncJob = LIBXL_ASYNC_JOB_NONE;
+ job->asyncOwner = 0;
+ job->phase = 0;
+ job->mask = DEFAULT_JOB_MASK;
+ job->start = 0;
+ memset(&job->info, 0, sizeof(job->info));
+}
+
+static void
+libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv)
+{
+ ignore_value(virCondDestroy(&priv->job.cond));
+ ignore_value(virCondDestroy(&priv->job.asyncCond));
+}
+
+static bool
+libxlDomainTrackJob(enum libxlDomainJob job)
+{
+ return (LIBXL_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0;
+}
+
+static void
+libxlDomainObjSaveJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
+{
+ if (!virDomainObjIsActive(obj)) {
+ /* don't write the state file yet, it will be written once the domain
+ * gets activated */
+ return;
+ }
+
+ if (virDomainSaveStatus(driver->caps, driver->stateDir, obj) < 0)
+ VIR_WARN("Failed to save status on vm %s", obj->def->name);
+}
+
+static bool
+libxlDomainNestedJobAllowed(libxlDomainObjPrivatePtr priv, enum libxlDomainJob job)
+{
+ return !priv->job.asyncJob || (priv->job.mask & JOB_MASK(job)) != 0;
+}
+
+static void
+libxlDomainObjSetAsyncJobMask(virDomainObjPtr obj,
+ unsigned long long allowedJobs)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+
+ if (!priv->job.asyncJob)
+ return;
+
+ priv->job.mask = allowedJobs | JOB_MASK(LIBXL_JOB_DESTROY);
+}
+
+/* Give up waiting for mutex after 30 seconds */
+#define LIBXL_JOB_WAIT_TIME (1000ull * 30)
+
+/*
+ * obj must be locked before calling; driver_locked says if libxlDriverPrivatePtr
+ * is locked or not.
+ */
+static int ATTRIBUTE_NONNULL(1)
+libxlDomainObjBeginJobInternal(libxlDriverPrivatePtr driver,
+ bool driver_locked,
+ virDomainObjPtr obj,
+ enum libxlDomainJob job,
+ enum libxlDomainAsyncJob asyncJob)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+ unsigned long long now;
+ unsigned long long then;
+
+ if (virTimeMillisNow(&now) < 0)
+ return -1;
+ then = now + LIBXL_JOB_WAIT_TIME;
+
+ virObjectRef(obj);
+ if (driver_locked) {
+ libxlDriverUnlock(driver);
+ }
+
+retry:
+ while (!libxlDomainNestedJobAllowed(priv, job)) {
+ VIR_DEBUG("Wait async job condition for starting job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ if (virCondWaitUntil(&priv->job.asyncCond, &obj->lock, then) <
0)
+ goto error;
+ }
+
+ while (priv->job.active) {
+ VIR_DEBUG("Wait normal job condition for starting job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ if (virCondWaitUntil(&priv->job.cond, &obj->lock, then) < 0)
+ goto error;
+ }
+
+ /* No job is active but a new async job could have been started while obj
+ * was unlocked, so we need to recheck it. */
+ if (!libxlDomainNestedJobAllowed(priv, job))
+ goto retry;
+
+ libxlDomainObjResetJob(priv);
+
+ if (job != LIBXL_JOB_ASYNC) {
+ VIR_DEBUG("Starting job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ priv->job.active = job;
+ priv->job.owner = virThreadSelfID();
+ } else {
+ VIR_DEBUG("Starting async job: %s",
+ libxlDomainAsyncJobTypeToString(asyncJob));
+ libxlDomainObjResetAsyncJob(priv);
+ priv->job.asyncJob = asyncJob;
+ priv->job.asyncOwner = virThreadSelfID();
+ priv->job.start = now;
+ }
+
+ if (driver_locked) {
+ virDomainObjUnlock(obj);
+ libxlDriverLock(driver);
+ virDomainObjLock(obj);
+ }
+
+ if (libxlDomainTrackJob(job))
+ libxlDomainObjSaveJob(driver, obj);
+
+ return 0;
+
+error:
+ VIR_WARN("Cannot start job (%s, %s) for domain %s;"
+ " current job is (%s, %s) owned by (%d, %d)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(asyncJob),
+ obj->def->name,
+ libxlDomainJobTypeToString(priv->job.active),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob),
+ priv->job.owner, priv->job.asyncOwner);
+
+ if (errno == ETIMEDOUT)
+ virReportError(VIR_ERR_OPERATION_TIMEOUT,
+ "%s", _("cannot acquire state change lock"));
+ else
+ virReportSystemError(errno,
+ "%s", _("cannot acquire job mutex"));
+ if (driver_locked) {
+ virDomainObjUnlock(obj);
+ libxlDriverLock(driver);
+ virDomainObjLock(obj);
+ }
+ virObjectUnref(obj);
+ return -1;
+}
+
+/*
+ * obj must be locked before calling, libxlDriverPrivatePtr must NOT be locked
+ *
+ * This must be called by anything that will change the VM state
+ * in any way
+ *
+ * Upon successful return, the object will have its ref count increased,
+ * successful calls must be followed by EndJob eventually
+ */
+static int
+libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainJob job)
+{
+ return libxlDomainObjBeginJobInternal(driver, false, obj, job,
+ LIBXL_ASYNC_JOB_NONE);
+}
+
+static int ATTRIBUTE_UNUSED
+libxlDomainObjBeginAsyncJob(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainAsyncJob asyncJob)
+{
+ return libxlDomainObjBeginJobInternal(driver, false, obj, LIBXL_JOB_ASYNC,
+ asyncJob);
+}
+
+/*
+ * obj must be locked before calling. If libxlDriverPrivatePtr is passed, it
+ * MUST be locked; otherwise it MUST NOT be locked.
+ *
+ * This must be called by anything that will change the VM state
+ * in any way
+ *
+ * Upon successful return, the object will have its ref count increased,
+ * successful calls must be followed by EndJob eventually
+ */
+static int
+libxlDomainObjBeginJobWithDriver(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainJob job)
+{
+ if (job <= LIBXL_JOB_NONE || job >= LIBXL_JOB_ASYNC) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Attempt to start invalid job"));
+ return -1;
+ }
+
+ return libxlDomainObjBeginJobInternal(driver, true, obj, job,
+ LIBXL_ASYNC_JOB_NONE);
+}
+
+static int
+libxlDomainObjBeginAsyncJobWithDriver(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainAsyncJob asyncJob)
+{
+ return libxlDomainObjBeginJobInternal(driver, true, obj, LIBXL_JOB_ASYNC,
+ asyncJob);
+}
+
+/*
+ * obj must be locked before calling, libxlDriverPrivatePtr does not matter
+ *
+ * To be called after completing the work associated with the
+ * earlier libxlDomainBeginJob() call
+ *
+ * Returns remaining refcount on 'obj', maybe 0 to indicated it
+ * was deleted
+ */
+static bool
+libxlDomainObjEndJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+ enum libxlDomainJob job = priv->job.active;
+
+ VIR_DEBUG("Stopping job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+
+ libxlDomainObjResetJob(priv);
+ if (libxlDomainTrackJob(job))
+ libxlDomainObjSaveJob(driver, obj);
+ virCondSignal(&priv->job.cond);
+
+ return virObjectUnref(obj);
+}
+
+static bool
+libxlDomainObjEndAsyncJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+
+ VIR_DEBUG("Stopping async job: %s",
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+
+ libxlDomainObjResetAsyncJob(priv);
+ libxlDomainObjSaveJob(driver, obj);
+ virCondBroadcast(&priv->job.asyncCond);
+
+ return virObjectUnref(obj);
+}
+
+static int ATTRIBUTE_UNUSED
+libxlMigrationJobStart(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ enum libxlDomainAsyncJob job)
+{
+ libxlDomainObjPrivatePtr priv = vm->privateData;
+
+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, job) < 0)
+ return -1;
+
+ libxlDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK |
+ JOB_MASK(LIBXL_JOB_MIGRATION_OP));
+
+ priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
+
+ return 0;
+}
+
+static bool ATTRIBUTE_UNUSED
+libxlMigrationJobFinish(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
+{
+ return libxlDomainObjEndAsyncJob(driver, vm);
+}
+/* job function finish */
+
static void *
libxlDomainObjPrivateAlloc(void)
{
@@ -92,11 +423,18 @@ libxlDomainObjPrivateAlloc(void)
if (VIR_ALLOC(priv) < 0)
return NULL;
+ if (libxlDomainObjInitJob(priv) < 0)
+ goto error;
+
libxl_ctx_init(&priv->ctx, LIBXL_VERSION, libxl_driver->logger);
priv->waiterFD = -1;
priv->eventHdl = -1;
return priv;
+
+error:
+ VIR_FREE(priv);
+ return NULL;
}
static void
@@ -114,6 +452,7 @@ libxlDomainObjPrivateFree(void *data)
}
libxl_ctx_free(&priv->ctx);
+ libxlDomainObjFreeJob(priv);
VIR_FREE(priv);
}
@@ -3830,6 +4169,111 @@ cleanup:
}
static int
+libxlDomainGetJobInfo(virDomainPtr dom,
+ virDomainJobInfoPtr info)
+{
+ libxlDriverPrivatePtr driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ libxlDomainObjPrivatePtr priv;
+
+ libxlDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ libxlDriverUnlock(driver);
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
+ priv = vm->privateData;
+
+ if (virDomainObjIsActive(vm)) {
+ if (priv->job.asyncJob) {
+ memcpy(info, &priv->job.info, sizeof(*info));
+
+ /* Refresh elapsed time again just to ensure it
+ * is fully updated. This is primarily for benefit
+ * of incoming migration which we don't currently
+ * monitor actively in the background thread
+ */
+ if (virTimeMillisNow(&info->timeElapsed) < 0)
+ goto cleanup;
+ info->timeElapsed -= priv->job.start;
+ } else {
+ memset(info, 0, sizeof(*info));
+ info->type = VIR_DOMAIN_JOB_NONE;
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
+ return ret;
+}
+
+static int
+libxlDomainAbortJob(virDomainPtr dom)
+{
+ libxlDriverPrivatePtr driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ libxlDomainObjPrivatePtr priv;
+
+ libxlDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ libxlDriverUnlock(driver);
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_ABORT) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ priv = vm->privateData;
+
+ if (!priv->job.asyncJob) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("no job is active on the domain"));
+ goto endjob;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("cannot abort %s; use virDomainDestroy instead"),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ goto endjob;
+ }
+
+ VIR_DEBUG("Not job could be cancelled at current version");
+
+endjob:
+ if (libxlDomainObjEndJob(driver, vm) == 0)
+ vm = NULL;
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
+ return ret;
+}
+
+static int
libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID,
virConnectDomainEventGenericCallback callback,
void *opaque, virFreeCallback freecb)
@@ -3963,6 +4407,8 @@ static virDriver libxlDriver = {
.domainIsActive = libxlDomainIsActive, /* 0.9.0 */
.domainIsPersistent = libxlDomainIsPersistent, /* 0.9.0 */
.domainIsUpdated = libxlDomainIsUpdated, /* 0.9.0 */
+ .domainGetJobInfo = libxlDomainGetJobInfo, /* 0.10.0 */
+ .domainAbortJob = libxlDomainAbortJob, /* 0.10.0 */
.domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */
.domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */
.isAlive = libxlIsAlive, /* 0.9.8 */
--
1.7.12
5:22 p.m.
New subject: [libvirt] [PATCH 1/2] Introduce a lock for libxl long-running api
Bamvor Jian Zhang wrote:
This patch introduce a lock for protecting the long-running
api (save, dump, migration and so on) from the other api
which may update the status of the virtual machine.
Hi Bamvor,
Thanks for the patches and sorry for the delayed response. I've been
traveling quite a bit lately and just got around to reviewing and
testing your work.
Testing so far looks good. I can save and dump vm's while at the same
time list and retrieve info.
See my comments inline, but did want to raise a more general comment
first. There is a quite a bit of code here borrowed from the qemu
driver, which in general is fine since the libxl driver does not need
the same locking features as the qemu one. I'd like to hear the opinion
of other libvirt maintainers wrt the duplicated code.
Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
---
src/libxl/libxl_conf.h | 58 ++++++
src/libxl/libxl_driver.c | 446 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 504 insertions(+)
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 56bf85c..dab1466 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -73,6 +73,62 @@ struct _libxlDriverPrivate {
char *saveDir;
};
+# define JOB_MASK(job) (1 << (job - 1))
+# define DEFAULT_JOB_MASK \
+ (JOB_MASK(LIBXL_JOB_DESTROY) | \
+ JOB_MASK(LIBXL_JOB_ABORT))
+
+/* Jobs which have to be tracked in domain state XML. */
+# define LIBXL_DOMAIN_TRACK_JOBS \
+ (JOB_MASK(LIBXL_JOB_DESTROY) | \
+ JOB_MASK(LIBXL_JOB_ASYNC))
+
+/* Only 1 job is allowed at any time
+ * A job includes *all* libxl.so api, even those just querying
+ * information, not merely actions */
+enum libxlDomainJob {
+ LIBXL_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive) conditions */
+ LIBXL_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */
+ LIBXL_JOB_MODIFY, /* May change state */
+ LIBXL_JOB_ABORT, /* Abort current async job */
+ LIBXL_JOB_MIGRATION_OP, /* Operation influencing outgoing migration */
+
+ /* The following two items must always be the last items before JOB_LAST */
+ LIBXL_JOB_ASYNC, /* Asynchronous job */
There is only one item, so the comment needs updated.
+
+ LIBXL_JOB_LAST
+};
+VIR_ENUM_DECL(libxlDomainJob)
+
+/* Async job consists of a series of jobs that may change state. Independent
+ * jobs that do not change state (and possibly others if explicitly allowed by
+ * current async job) are allowed to be run even if async job is active.
+ */
+enum libxlDomainAsyncJob {
+ LIBXL_ASYNC_JOB_NONE = 0,
+ LIBXL_ASYNC_JOB_MIGRATION_OUT,
+ LIBXL_ASYNC_JOB_MIGRATION_IN,
The libxl driver doesn't currently support migration. These could be
added in a patch set implementing migration.
+ LIBXL_ASYNC_JOB_SAVE,
+ LIBXL_ASYNC_JOB_DUMP,
IMO, LIBXL_ASYNC_JOB_RESTORE should be added to this list. I see It is
omitted in the qemu driver as well, so perhaps there is a good reason
for it?
+
+ LIBXL_ASYNC_JOB_LAST
+};
+VIR_ENUM_DECL(libxlDomainAsyncJob)
+
+struct libxlDomainJobObj {
+ virCond cond; /* Use to coordinate jobs */
+ enum libxlDomainJob active; /* Currently running job */
+ int owner; /* Thread which set current job */
+
+ virCond asyncCond; /* Use to coordinate with async jobs */
+ enum libxlDomainAsyncJob asyncJob; /* Currently active async job */
+ int asyncOwner; /* Thread which set current async job */
+ int phase; /* Job phase (mainly for migrations) */
+ unsigned long long mask; /* Jobs allowed during async job */
+ unsigned long long start; /* When the async job started */
+ virDomainJobInfo info; /* Async job progress data */
+};
+
typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate;
typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
struct _libxlDomainObjPrivate {
@@ -81,6 +137,8 @@ struct _libxlDomainObjPrivate {
libxl_waiter *dWaiter;
int waiterFD;
int eventHdl;
+
+ struct libxlDomainJobObj job;
};
# define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 6fe284f..d01d915 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -45,6 +45,7 @@
#include "xen_xm.h"
#include "virtypedparam.h"
#include "viruri.h"
+#include "virtime.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -84,6 +85,336 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver)
virMutexUnlock(&driver->lock);
}
+/* job */
+VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST,
+ "none",
+ "destroy",
+ "modify",
+ "abort",
+ "migration operation",
+ "none", /* async job is never stored in job.active */
+);
+
+VIR_ENUM_IMPL(libxlDomainAsyncJob, LIBXL_ASYNC_JOB_LAST,
+ "none",
+ "migration out",
+ "migration in",
Can be added with a migration implementation.
+ "save",
+ "dump",
"restore"?
+);
+
+static int
+libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
+{
+ memset(&priv->job, 0, sizeof(priv->job));
+
+ if (virCondInit(&priv->job.cond) < 0)
+ return -1;
+
+ if (virCondInit(&priv->job.asyncCond) < 0) {
+ ignore_value(virCondDestroy(&priv->job.cond));
+ return -1;
+ }
+
+ return 0;
+}
+
+static void
+libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv)
+{
+ struct libxlDomainJobObj *job = &priv->job;
+
+ job->active = LIBXL_JOB_NONE;
+ job->owner = 0;
+}
+
+static void
+libxlDomainObjResetAsyncJob(libxlDomainObjPrivatePtr priv)
+{
+ struct libxlDomainJobObj *job = &priv->job;
+
+ job->asyncJob = LIBXL_ASYNC_JOB_NONE;
+ job->asyncOwner = 0;
+ job->phase = 0;
+ job->mask = DEFAULT_JOB_MASK;
+ job->start = 0;
+ memset(&job->info, 0, sizeof(job->info));
+}
+
+static void
+libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv)
+{
+ ignore_value(virCondDestroy(&priv->job.cond));
+ ignore_value(virCondDestroy(&priv->job.asyncCond));
+}
+
+static bool
+libxlDomainTrackJob(enum libxlDomainJob job)
+{
+ return (LIBXL_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0;
+}
+
+static void
+libxlDomainObjSaveJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
+{
+ if (!virDomainObjIsActive(obj)) {
+ /* don't write the state file yet, it will be written once the domain
+ * gets activated */
+ return;
+ }
+
+ if (virDomainSaveStatus(driver->caps, driver->stateDir, obj) < 0)
+ VIR_WARN("Failed to save status on vm %s", obj->def->name);
+}
+
+static bool
+libxlDomainNestedJobAllowed(libxlDomainObjPrivatePtr priv, enum libxlDomainJob job)
+{
+ return !priv->job.asyncJob || (priv->job.mask & JOB_MASK(job)) != 0;
+}
+
+static void
+libxlDomainObjSetAsyncJobMask(virDomainObjPtr obj,
+ unsigned long long allowedJobs)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+
+ if (!priv->job.asyncJob)
+ return;
+
+ priv->job.mask = allowedJobs | JOB_MASK(LIBXL_JOB_DESTROY);
+}
+
+/* Give up waiting for mutex after 30 seconds */
+#define LIBXL_JOB_WAIT_TIME (1000ull * 30)
+
+/*
+ * obj must be locked before calling; driver_locked says if libxlDriverPrivatePtr
+ * is locked or not.
+ */
+static int ATTRIBUTE_NONNULL(1)
+libxlDomainObjBeginJobInternal(libxlDriverPrivatePtr driver,
+ bool driver_locked,
+ virDomainObjPtr obj,
+ enum libxlDomainJob job,
+ enum libxlDomainAsyncJob asyncJob)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+ unsigned long long now;
+ unsigned long long then;
+
+ if (virTimeMillisNow(&now) < 0)
+ return -1;
+ then = now + LIBXL_JOB_WAIT_TIME;
+
+ virObjectRef(obj);
+ if (driver_locked) {
+ libxlDriverUnlock(driver);
+ }
+
+retry:
+ while (!libxlDomainNestedJobAllowed(priv, job)) {
+ VIR_DEBUG("Wait async job condition for starting job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ if (virCondWaitUntil(&priv->job.asyncCond, &obj->lock, then) <
0)
+ goto error;
+ }
+
+ while (priv->job.active) {
+ VIR_DEBUG("Wait normal job condition for starting job: %s
(async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ if (virCondWaitUntil(&priv->job.cond, &obj->lock, then) < 0)
+ goto error;
+ }
+
+ /* No job is active but a new async job could have been started while obj
+ * was unlocked, so we need to recheck it. */
+ if (!libxlDomainNestedJobAllowed(priv, job))
+ goto retry;
+
+ libxlDomainObjResetJob(priv);
+
+ if (job != LIBXL_JOB_ASYNC) {
+ VIR_DEBUG("Starting job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ priv->job.active = job;
+ priv->job.owner = virThreadSelfID();
+ } else {
+ VIR_DEBUG("Starting async job: %s",
+ libxlDomainAsyncJobTypeToString(asyncJob));
+ libxlDomainObjResetAsyncJob(priv);
+ priv->job.asyncJob = asyncJob;
+ priv->job.asyncOwner = virThreadSelfID();
+ priv->job.start = now;
+ }
+
+ if (driver_locked) {
+ virDomainObjUnlock(obj);
+ libxlDriverLock(driver);
+ virDomainObjLock(obj);
+ }
+
+ if (libxlDomainTrackJob(job))
+ libxlDomainObjSaveJob(driver, obj);
+
+ return 0;
+
+error:
+ VIR_WARN("Cannot start job (%s, %s) for domain %s;"
+ " current job is (%s, %s) owned by (%d, %d)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(asyncJob),
+ obj->def->name,
+ libxlDomainJobTypeToString(priv->job.active),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob),
+ priv->job.owner, priv->job.asyncOwner);
+
+ if (errno == ETIMEDOUT)
+ virReportError(VIR_ERR_OPERATION_TIMEOUT,
+ "%s", _("cannot acquire state change
lock"));
+ else
+ virReportSystemError(errno,
+ "%s", _("cannot acquire job mutex"));
+ if (driver_locked) {
+ virDomainObjUnlock(obj);
+ libxlDriverLock(driver);
+ virDomainObjLock(obj);
+ }
+ virObjectUnref(obj);
+ return -1;
+}
+
+/*
+ * obj must be locked before calling, libxlDriverPrivatePtr must NOT be locked
+ *
+ * This must be called by anything that will change the VM state
+ * in any way
+ *
+ * Upon successful return, the object will have its ref count increased,
+ * successful calls must be followed by EndJob eventually
+ */
+static int
+libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainJob job)
+{
+ return libxlDomainObjBeginJobInternal(driver, false, obj, job,
+ LIBXL_ASYNC_JOB_NONE);
+}
+
+static int ATTRIBUTE_UNUSED
+libxlDomainObjBeginAsyncJob(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainAsyncJob asyncJob)
+{
+ return libxlDomainObjBeginJobInternal(driver, false, obj, LIBXL_JOB_ASYNC,
+ asyncJob);
+}
+
+/*
+ * obj must be locked before calling. If libxlDriverPrivatePtr is passed, it
+ * MUST be locked; otherwise it MUST NOT be locked.
IMO, libxlDriverPrivatePtr should always be passed, in which case it
MUST be locked.
+ *
+ * This must be called by anything that will change the VM state
+ * in any way
+ *
+ * Upon successful return, the object will have its ref count increased,
+ * successful calls must be followed by EndJob eventually
+ */
+static int
+libxlDomainObjBeginJobWithDriver(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainJob job)
+{
+ if (job <= LIBXL_JOB_NONE || job >= LIBXL_JOB_ASYNC) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Attempt to start invalid job"));
+ return -1;
+ }
+
+ return libxlDomainObjBeginJobInternal(driver, true, obj, job,
+ LIBXL_ASYNC_JOB_NONE);
+}
+
+static int
+libxlDomainObjBeginAsyncJobWithDriver(libxlDriverPrivatePtr driver,
+ virDomainObjPtr obj,
+ enum libxlDomainAsyncJob asyncJob)
+{
+ return libxlDomainObjBeginJobInternal(driver, true, obj, LIBXL_JOB_ASYNC,
+ asyncJob);
+}
+
+/*
+ * obj must be locked before calling, libxlDriverPrivatePtr does not matter
+ *
+ * To be called after completing the work associated with the
+ * earlier libxlDomainBeginJob() call
+ *
+ * Returns remaining refcount on 'obj', maybe 0 to indicated it
+ * was deleted
+ */
+static bool
+libxlDomainObjEndJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+ enum libxlDomainJob job = priv->job.active;
+
+ VIR_DEBUG("Stopping job: %s (async=%s)",
+ libxlDomainJobTypeToString(job),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+
+ libxlDomainObjResetJob(priv);
+ if (libxlDomainTrackJob(job))
+ libxlDomainObjSaveJob(driver, obj);
+ virCondSignal(&priv->job.cond);
+
+ return virObjectUnref(obj);
+}
+
+static bool
+libxlDomainObjEndAsyncJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
+{
+ libxlDomainObjPrivatePtr priv = obj->privateData;
+
+ VIR_DEBUG("Stopping async job: %s",
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+
+ libxlDomainObjResetAsyncJob(priv);
+ libxlDomainObjSaveJob(driver, obj);
+ virCondBroadcast(&priv->job.asyncCond);
+
+ return virObjectUnref(obj);
+}
+
+static int ATTRIBUTE_UNUSED
+libxlMigrationJobStart(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ enum libxlDomainAsyncJob job)
This function is not used in the patch series and should be removed.
+{
+ libxlDomainObjPrivatePtr priv = vm->privateData;
+
+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, job) < 0)
+ return -1;
+
+ libxlDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK |
+ JOB_MASK(LIBXL_JOB_MIGRATION_OP));
+
+ priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
+
+ return 0;
+}
+
+static bool ATTRIBUTE_UNUSED
+libxlMigrationJobFinish(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
Same here, not used so should be removed.
+{
+ return libxlDomainObjEndAsyncJob(driver, vm);
+}
+/* job function finish */
+
static void *
libxlDomainObjPrivateAlloc(void)
{
@@ -92,11 +423,18 @@ libxlDomainObjPrivateAlloc(void)
if (VIR_ALLOC(priv) < 0)
return NULL;
+ if (libxlDomainObjInitJob(priv) < 0)
+ goto error;
+
libxl_ctx_init(&priv->ctx, LIBXL_VERSION, libxl_driver->logger);
priv->waiterFD = -1;
priv->eventHdl = -1;
return priv;
+
+error:
+ VIR_FREE(priv);
+ return NULL;
}
static void
@@ -114,6 +452,7 @@ libxlDomainObjPrivateFree(void *data)
}
libxl_ctx_free(&priv->ctx);
+ libxlDomainObjFreeJob(priv);
VIR_FREE(priv);
}
@@ -3830,6 +4169,111 @@ cleanup:
}
static int
+libxlDomainGetJobInfo(virDomainPtr dom,
+ virDomainJobInfoPtr info)
+{
+ libxlDriverPrivatePtr driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ libxlDomainObjPrivatePtr priv;
+
+ libxlDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ libxlDriverUnlock(driver);
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
+ priv = vm->privateData;
+
+ if (virDomainObjIsActive(vm)) {
+ if (priv->job.asyncJob) {
+ memcpy(info, &priv->job.info, sizeof(*info));
+
+ /* Refresh elapsed time again just to ensure it
+ * is fully updated. This is primarily for benefit
+ * of incoming migration which we don't currently
+ * monitor actively in the background thread
+ */
+ if (virTimeMillisNow(&info->timeElapsed) < 0)
+ goto cleanup;
+ info->timeElapsed -= priv->job.start;
+ } else {
+ memset(info, 0, sizeof(*info));
+ info->type = VIR_DOMAIN_JOB_NONE;
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
+ return ret;
+}
+
+static int
+libxlDomainAbortJob(virDomainPtr dom)
+{
+ libxlDriverPrivatePtr driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ libxlDomainObjPrivatePtr priv;
+
+ libxlDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ libxlDriverUnlock(driver);
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_ABORT) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ priv = vm->privateData;
+
+ if (!priv->job.asyncJob) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("no job is active on the domain"));
+ goto endjob;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("cannot abort %s; use virDomainDestroy instead"),
+ libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
+ goto endjob;
+ }
This function will always fail with the above logic. ret is initialized
to -1 and is never changed.
Is it even possible to safely abort a libxl operation? If not, this
function should probably remain unimplemented. Maybe it will be useful
when the libxl driver supports migration.
Regards,
Jim
+
+ VIR_DEBUG("Not job could be cancelled at current version");
+
+endjob:
+ if (libxlDomainObjEndJob(driver, vm) == 0)
+ vm = NULL;
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
+ return ret;
+}
+
+static int
libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID,
virConnectDomainEventGenericCallback callback,
void *opaque, virFreeCallback freecb)
@@ -3963,6 +4407,8 @@ static virDriver libxlDriver = {
.domainIsActive = libxlDomainIsActive, /* 0.9.0 */
.domainIsPersistent = libxlDomainIsPersistent, /* 0.9.0 */
.domainIsUpdated = libxlDomainIsUpdated, /* 0.9.0 */
+ .domainGetJobInfo = libxlDomainGetJobInfo, /* 0.10.0 */
+ .domainAbortJob = libxlDomainAbortJob, /* 0.10.0 */
.domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */
.domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */
.isAlive = libxlIsAlive, /* 0.9.8 */
10:33 a.m.
New subject: [libvirt] [PATCH 1/2] Introduce a lock for libxl long-running api
On Mon, Oct 22, 2012 at 04:22:53PM -0600, Jim Fehlig wrote:
Bamvor Jian Zhang wrote:
> This patch introduce a lock for protecting the long-running
> api (save, dump, migration and so on) from the other api
> which may update the status of the virtual machine.
>
Hi Bamvor,
Thanks for the patches and sorry for the delayed response. I've been
traveling quite a bit lately and just got around to reviewing and
testing your work.
Testing so far looks good. I can save and dump vm's while at the same
time list and retrieve info.
See my comments inline, but did want to raise a more general comment
first. There is a quite a bit of code here borrowed from the qemu
driver, which in general is fine since the libxl driver does not need
the same locking features as the qemu one. I'd like to hear the opinion
of other libvirt maintainers wrt the duplicated code.
I was actually quite pleased to see that the libxl driver was following
the model used in the QEMU driver, even though there was code copying.
I don't think it is worth trying to eliminate the code duplication in
this case.
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 :|
3:32 a.m.
New subject: [libvirt] [PATCH 1/2] Introduce a lock for libxl long-running api
Hi, Jim
thanks your reply.
i have update my patch and resend to the list.
besides, there are two comments from you not modify in my v2 patch.
see my comments below.
>>Jim Fehlig <jfehlig(a)suse.com> wrote:
Bamvor Jian Zhang wrote:
> This patch introduce a lock for protecting the long-running
> api (save, dump, migration and so on) from the other api
> which may update the status of the virtual machine.
>
Hi Bamvor,
Thanks for the patches and sorry for the delayed response. I've been
traveling quite a bit lately and just got around to reviewing and
testing your work.
Testing so far looks good. I can save and dump vm's while at the same
time list and retrieve info.
See my comments inline, but did want to raise a more general comment
first. There is a quite a bit of code here borrowed from the qemu
driver, which in general is fine since the libxl driver does not need
the same locking features as the qemu one. I'd like to hear the opinion
of other libvirt maintainers wrt the duplicated code.
> Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
> ---
> src/libxl/libxl_conf.h | 58 ++++++
> src/libxl/libxl_driver.c | 446
+++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 504 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 56bf85c..dab1466 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -73,6 +73,62 @@ struct _libxlDriverPrivate {
> char *saveDir;
> };
>
> +# define JOB_MASK(job) (1 << (job - 1))
> +# define DEFAULT_JOB_MASK \
> + (JOB_MASK(LIBXL_JOB_DESTROY) | \
> + JOB_MASK(LIBXL_JOB_ABORT))
> +
> +/* Jobs which have to be tracked in domain state XML. */
> +# define LIBXL_DOMAIN_TRACK_JOBS \
> + (JOB_MASK(LIBXL_JOB_DESTROY) | \
> + JOB_MASK(LIBXL_JOB_ASYNC))
> +
> +/* Only 1 job is allowed at any time
> + * A job includes *all* libxl.so api, even those just querying
> + * information, not merely actions */
> +enum libxlDomainJob {
> + LIBXL_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive)
conditions */
> + LIBXL_JOB_DESTROY, /* Destroys the domain (cannot be masked out)
*/
> + LIBXL_JOB_MODIFY, /* May change state */
> + LIBXL_JOB_ABORT, /* Abort current async job */
> + LIBXL_JOB_MIGRATION_OP, /* Operation influencing outgoing migration
*/
> +
> + /* The following two items must always be the last items before
JOB_LAST */
> + LIBXL_JOB_ASYNC, /* Asynchronous job */
>
There is only one item, so the comment needs updated.
> +
> + LIBXL_JOB_LAST
> +};
> +VIR_ENUM_DECL(libxlDomainJob)
> +
> +/* Async job consists of a series of jobs that may change state.
Independent
> + * jobs that do not change state (and possibly others if explicitly
allowed by
> + * current async job) are allowed to be run even if async job is active.
> + */
> +enum libxlDomainAsyncJob {
> + LIBXL_ASYNC_JOB_NONE = 0,
> + LIBXL_ASYNC_JOB_MIGRATION_OUT,
> + LIBXL_ASYNC_JOB_MIGRATION_IN,
>
The libxl driver doesn't currently support migration. These could be
added in a patch set implementing migration.
> + LIBXL_ASYNC_JOB_SAVE,
> + LIBXL_ASYNC_JOB_DUMP,
>
IMO, LIBXL_ASYNC_JOB_RESTORE should be added to this list. I see It is
omitted in the qemu driver as well, so perhaps there is a good reason
for it?
i do not know why it is omitted by qemu driver. but for libxl driver, it
is
hard to let other operation execution even if it is a async job. because
virDomainObjPtr is used by libxlVmStart, especially by
libxl_domain_create_restore.
> +
> + LIBXL_ASYNC_JOB_LAST
> +};
> +VIR_ENUM_DECL(libxlDomainAsyncJob)
> +
> +struct libxlDomainJobObj {
> + virCond cond; /* Use to coordinate jobs */
> + enum libxlDomainJob active; /* Currently running job */
> + int owner; /* Thread which set current job */
> +
> + virCond asyncCond; /* Use to coordinate with async
jobs */
> + enum libxlDomainAsyncJob asyncJob; /* Currently active async job */
> + int asyncOwner; /* Thread which set current async
job */
> + int phase; /* Job phase (mainly for
migrations) */
> + unsigned long long mask; /* Jobs allowed during async job
*/
> + unsigned long long start; /* When the async job started */
> + virDomainJobInfo info; /* Async job progress data */
> +};
> +
> typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate;
> typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
> struct _libxlDomainObjPrivate {
> @@ -81,6 +137,8 @@ struct _libxlDomainObjPrivate {
> libxl_waiter *dWaiter;
> int waiterFD;
> int eventHdl;
> +
> + struct libxlDomainJobObj job;
> };
>
> # define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 6fe284f..d01d915 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -45,6 +45,7 @@
> #include "xen_xm.h"
> #include "virtypedparam.h"
> #include "viruri.h"
> +#include "virtime.h"
>
> #define VIR_FROM_THIS VIR_FROM_LIBXL
>
> @@ -84,6 +85,336 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver)
> virMutexUnlock(&driver->lock);
> }
>
> +/* job */
> +VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST,
> + "none",
> + "destroy",
> + "modify",
> + "abort",
> + "migration operation",
> + "none", /* async job is never stored in job.active */
> +);
> +
> +VIR_ENUM_IMPL(libxlDomainAsyncJob, LIBXL_ASYNC_JOB_LAST,
> + "none",
> + "migration out",
> + "migration in",
>
Can be added with a migration implementation.
> + "save",
> + "dump",
>
"restore"?
> +);
> +
> +static int
> +libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
> +{
> + memset(&priv->job, 0, sizeof(priv->job));
> +
> + if (virCondInit(&priv->job.cond) < 0)
> + return -1;
> +
> + if (virCondInit(&priv->job.asyncCond) < 0) {
> + ignore_value(virCondDestroy(&priv->job.cond));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv)
> +{
> + struct libxlDomainJobObj *job = &priv->job;
> +
> + job->active = LIBXL_JOB_NONE;
> + job->owner = 0;
> +}
> +
> +static void
> +libxlDomainObjResetAsyncJob(libxlDomainObjPrivatePtr priv)
> +{
> + struct libxlDomainJobObj *job = &priv->job;
> +
> + job->asyncJob = LIBXL_ASYNC_JOB_NONE;
> + job->asyncOwner = 0;
> + job->phase = 0;
> + job->mask = DEFAULT_JOB_MASK;
> + job->start = 0;
> + memset(&job->info, 0, sizeof(job->info));
> +}
> +
> +static void
> +libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv)
> +{
> + ignore_value(virCondDestroy(&priv->job.cond));
> + ignore_value(virCondDestroy(&priv->job.asyncCond));
> +}
> +
> +static bool
> +libxlDomainTrackJob(enum libxlDomainJob job)
> +{
> + return (LIBXL_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0;
> +}
> +
> +static void
> +libxlDomainObjSaveJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
> +{
> + if (!virDomainObjIsActive(obj)) {
> + /* don't write the state file yet, it will be written once the
domain
> + * gets activated */
> + return;
> + }
> +
> + if (virDomainSaveStatus(driver->caps, driver->stateDir, obj) < 0)
> + VIR_WARN("Failed to save status on vm %s", obj->def->name);
> +}
> +
> +static bool
> +libxlDomainNestedJobAllowed(libxlDomainObjPrivatePtr priv, enum
libxlDomainJob job)
> +{
> + return !priv->job.asyncJob || (priv->job.mask & JOB_MASK(job)) != 0;
> +}
> +
> +static void
> +libxlDomainObjSetAsyncJobMask(virDomainObjPtr obj,
> + unsigned long long allowedJobs)
> +{
> + libxlDomainObjPrivatePtr priv = obj->privateData;
> +
> + if (!priv->job.asyncJob)
> + return;
> +
> + priv->job.mask = allowedJobs | JOB_MASK(LIBXL_JOB_DESTROY);
> +}
> +
> +/* Give up waiting for mutex after 30 seconds */
> +#define LIBXL_JOB_WAIT_TIME (1000ull * 30)
> +
> +/*
> + * obj must be locked before calling; driver_locked says if
libxlDriverPrivatePtr
> + * is locked or not.
> + */
> +static int ATTRIBUTE_NONNULL(1)
> +libxlDomainObjBeginJobInternal(libxlDriverPrivatePtr driver,
> + bool driver_locked,
> + virDomainObjPtr obj,
> + enum libxlDomainJob job,
> + enum libxlDomainAsyncJob asyncJob)
> +{
> + libxlDomainObjPrivatePtr priv = obj->privateData;
> + unsigned long long now;
> + unsigned long long then;
> +
> + if (virTimeMillisNow(&now) < 0)
> + return -1;
> + then = now + LIBXL_JOB_WAIT_TIME;
> +
> + virObjectRef(obj);
> + if (driver_locked) {
> + libxlDriverUnlock(driver);
> + }
> +
> +retry:
> + while (!libxlDomainNestedJobAllowed(priv, job)) {
> + VIR_DEBUG("Wait async job condition for starting job: %s
(async=%s)",
> + libxlDomainJobTypeToString(job),
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
> + if (virCondWaitUntil(&priv->job.asyncCond, &obj->lock, then)
< 0)
> + goto error;
> + }
> +
> + while (priv->job.active) {
> + VIR_DEBUG("Wait normal job condition for starting job: %s
(async=%s)",
> + libxlDomainJobTypeToString(job),
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
> + if (virCondWaitUntil(&priv->job.cond, &obj->lock, then) <
0)
> + goto error;
> + }
> +
> + /* No job is active but a new async job could have been started while
obj
> + * was unlocked, so we need to recheck it. */
> + if (!libxlDomainNestedJobAllowed(priv, job))
> + goto retry;
> +
> + libxlDomainObjResetJob(priv);
> +
> + if (job != LIBXL_JOB_ASYNC) {
> + VIR_DEBUG("Starting job: %s (async=%s)",
> + libxlDomainJobTypeToString(job),
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
> + priv->job.active = job;
> + priv->job.owner = virThreadSelfID();
> + } else {
> + VIR_DEBUG("Starting async job: %s",
> + libxlDomainAsyncJobTypeToString(asyncJob));
> + libxlDomainObjResetAsyncJob(priv);
> + priv->job.asyncJob = asyncJob;
> + priv->job.asyncOwner = virThreadSelfID();
> + priv->job.start = now;
> + }
> +
> + if (driver_locked) {
> + virDomainObjUnlock(obj);
> + libxlDriverLock(driver);
> + virDomainObjLock(obj);
> + }
> +
> + if (libxlDomainTrackJob(job))
> + libxlDomainObjSaveJob(driver, obj);
> +
> + return 0;
> +
> +error:
> + VIR_WARN("Cannot start job (%s, %s) for domain %s;"
> + " current job is (%s, %s) owned by (%d, %d)",
> + libxlDomainJobTypeToString(job),
> + libxlDomainAsyncJobTypeToString(asyncJob),
> + obj->def->name,
> + libxlDomainJobTypeToString(priv->job.active),
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob),
> + priv->job.owner, priv->job.asyncOwner);
> +
> + if (errno == ETIMEDOUT)
> + virReportError(VIR_ERR_OPERATION_TIMEOUT,
> + "%s", _("cannot acquire state change
lock"));
> + else
> + virReportSystemError(errno,
> + "%s", _("cannot acquire job
mutex"));
> + if (driver_locked) {
> + virDomainObjUnlock(obj);
> + libxlDriverLock(driver);
> + virDomainObjLock(obj);
> + }
> + virObjectUnref(obj);
> + return -1;
> +}
> +
> +/*
> + * obj must be locked before calling, libxlDriverPrivatePtr must NOT be
locked
> + *
> + * This must be called by anything that will change the VM state
> + * in any way
> + *
> + * Upon successful return, the object will have its ref count increased,
> + * successful calls must be followed by EndJob eventually
> + */
> +static int
> +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
> + virDomainObjPtr obj,
> + enum libxlDomainJob job)
> +{
> + return libxlDomainObjBeginJobInternal(driver, false, obj, job,
> + LIBXL_ASYNC_JOB_NONE);
> +}
> +
> +static int ATTRIBUTE_UNUSED
> +libxlDomainObjBeginAsyncJob(libxlDriverPrivatePtr driver,
> + virDomainObjPtr obj,
> + enum libxlDomainAsyncJob asyncJob)
> +{
> + return libxlDomainObjBeginJobInternal(driver, false, obj,
LIBXL_JOB_ASYNC,
> + asyncJob);
> +}
> +
> +/*
> + * obj must be locked before calling. If libxlDriverPrivatePtr is passed,
it
> + * MUST be locked; otherwise it MUST NOT be locked.
>
IMO, libxlDriverPrivatePtr should always be passed, in which case it
MUST be locked.
> + *
> + * This must be called by anything that will change the VM state
> + * in any way
> + *
> + * Upon successful return, the object will have its ref count increased,
> + * successful calls must be followed by EndJob eventually
> + */
> +static int
> +libxlDomainObjBeginJobWithDriver(libxlDriverPrivatePtr driver,
> + virDomainObjPtr obj,
> + enum libxlDomainJob job)
> +{
> + if (job <= LIBXL_JOB_NONE || job >= LIBXL_JOB_ASYNC) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Attempt to start invalid job"));
> + return -1;
> + }
> +
> + return libxlDomainObjBeginJobInternal(driver, true, obj, job,
> + LIBXL_ASYNC_JOB_NONE);
> +}
> +
> +static int
> +libxlDomainObjBeginAsyncJobWithDriver(libxlDriverPrivatePtr driver,
> + virDomainObjPtr obj,
> + enum libxlDomainAsyncJob asyncJob)
> +{
> + return libxlDomainObjBeginJobInternal(driver, true, obj,
LIBXL_JOB_ASYNC,
> + asyncJob);
> +}
> +
> +/*
> + * obj must be locked before calling, libxlDriverPrivatePtr does not
matter
> + *
> + * To be called after completing the work associated with the
> + * earlier libxlDomainBeginJob() call
> + *
> + * Returns remaining refcount on 'obj', maybe 0 to indicated it
> + * was deleted
> + */
> +static bool
> +libxlDomainObjEndJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj)
> +{
> + libxlDomainObjPrivatePtr priv = obj->privateData;
> + enum libxlDomainJob job = priv->job.active;
> +
> + VIR_DEBUG("Stopping job: %s (async=%s)",
> + libxlDomainJobTypeToString(job),
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
> +
> + libxlDomainObjResetJob(priv);
> + if (libxlDomainTrackJob(job))
> + libxlDomainObjSaveJob(driver, obj);
> + virCondSignal(&priv->job.cond);
> +
> + return virObjectUnref(obj);
> +}
> +
> +static bool
> +libxlDomainObjEndAsyncJob(libxlDriverPrivatePtr driver, virDomainObjPtr
obj)
> +{
> + libxlDomainObjPrivatePtr priv = obj->privateData;
> +
> + VIR_DEBUG("Stopping async job: %s",
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
> +
> + libxlDomainObjResetAsyncJob(priv);
> + libxlDomainObjSaveJob(driver, obj);
> + virCondBroadcast(&priv->job.asyncCond);
> +
> + return virObjectUnref(obj);
> +}
> +
> +static int ATTRIBUTE_UNUSED
> +libxlMigrationJobStart(libxlDriverPrivatePtr driver,
> + virDomainObjPtr vm,
> + enum libxlDomainAsyncJob job)
>
This function is not used in the patch series and should be removed.
> +{
> + libxlDomainObjPrivatePtr priv = vm->privateData;
> +
> + if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, job) < 0)
> + return -1;
> +
> + libxlDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK |
> + JOB_MASK(LIBXL_JOB_MIGRATION_OP));
> +
> + priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
> +
> + return 0;
> +}
> +
> +static bool ATTRIBUTE_UNUSED
> +libxlMigrationJobFinish(libxlDriverPrivatePtr driver, virDomainObjPtr vm)
>
Same here, not used so should be removed.
> +{
> + return libxlDomainObjEndAsyncJob(driver, vm);
> +}
> +/* job function finish */
> +
> static void *
> libxlDomainObjPrivateAlloc(void)
> {
> @@ -92,11 +423,18 @@ libxlDomainObjPrivateAlloc(void)
> if (VIR_ALLOC(priv) < 0)
> return NULL;
>
> + if (libxlDomainObjInitJob(priv) < 0)
> + goto error;
> +
> libxl_ctx_init(&priv->ctx, LIBXL_VERSION, libxl_driver->logger);
> priv->waiterFD = -1;
> priv->eventHdl = -1;
>
> return priv;
> +
> +error:
> + VIR_FREE(priv);
> + return NULL;
> }
>
> static void
> @@ -114,6 +452,7 @@ libxlDomainObjPrivateFree(void *data)
> }
>
> libxl_ctx_free(&priv->ctx);
> + libxlDomainObjFreeJob(priv);
> VIR_FREE(priv);
> }
>
> @@ -3830,6 +4169,111 @@ cleanup:
> }
>
> static int
> +libxlDomainGetJobInfo(virDomainPtr dom,
> + virDomainJobInfoPtr info)
> +{
> + libxlDriverPrivatePtr driver = dom->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> + libxlDomainObjPrivatePtr priv;
> +
> + libxlDriverLock(driver);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> + libxlDriverUnlock(driver);
> + if (!vm) {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virUUIDFormat(dom->uuid, uuidstr);
> + virReportError(VIR_ERR_NO_DOMAIN,
> + _("no domain with matching uuid '%s'"),
uuidstr);
> + goto cleanup;
> + }
> +
> + priv = vm->privateData;
> +
> + if (virDomainObjIsActive(vm)) {
> + if (priv->job.asyncJob) {
> + memcpy(info, &priv->job.info, sizeof(*info));
> +
> + /* Refresh elapsed time again just to ensure it
> + * is fully updated. This is primarily for benefit
> + * of incoming migration which we don't currently
> + * monitor actively in the background thread
> + */
> + if (virTimeMillisNow(&info->timeElapsed) < 0)
> + goto cleanup;
> + info->timeElapsed -= priv->job.start;
> + } else {
> + memset(info, 0, sizeof(*info));
> + info->type = VIR_DOMAIN_JOB_NONE;
> + }
> + } else {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + if (vm)
> + virDomainObjUnlock(vm);
> + return ret;
> +}
> +
> +static int
> +libxlDomainAbortJob(virDomainPtr dom)
> +{
> + libxlDriverPrivatePtr driver = dom->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> + libxlDomainObjPrivatePtr priv;
> +
> + libxlDriverLock(driver);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> + libxlDriverUnlock(driver);
> + if (!vm) {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virUUIDFormat(dom->uuid, uuidstr);
> + virReportError(VIR_ERR_NO_DOMAIN,
> + _("no domain with matching uuid '%s'"),
uuidstr);
> + goto cleanup;
> + }
> +
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_ABORT) < 0)
> + goto cleanup;
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto endjob;
> + }
> +
> + priv = vm->privateData;
> +
> + if (!priv->job.asyncJob) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("no job is active on the
domain"));
> + goto endjob;
> + } else {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("cannot abort %s; use virDomainDestroy
instead"),
> + libxlDomainAsyncJobTypeToString(priv->job.asyncJob));
> + goto endjob;
> + }
>
This function will always fail with the above logic. ret is initialized
to -1 and is never changed.
Is it even possible to safely abort a libxl operation? If not, this
function should probably remain unimplemented. Maybe it will be useful
when the libxl driver supports migration.
return error because of the there is no
cancelation opeartion in libvirt libxl
driver with xen 4.1. according to xen4.2 release document, maybe the
cancelation of long-running jobs is supported.
but it is still useful for save, dump and migration(in future), because libvirt
should block the user abort operation othervise xenlight might crash
Regards,
Jim
> +
> + VIR_DEBUG("Not job could be cancelled at current version");
> +
> +endjob:
> + if (libxlDomainObjEndJob(driver, vm) == 0)
> + vm = NULL;
> +
> +cleanup:
> + if (vm)
> + virDomainObjUnlock(vm);
> + return ret;
> +}
> +
> +static int
> libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int
eventID,
> virConnectDomainEventGenericCallback callback,
> void *opaque, virFreeCallback freecb)
> @@ -3963,6 +4407,8 @@ static virDriver libxlDriver = {
> .domainIsActive = libxlDomainIsActive, /* 0.9.0 */
> .domainIsPersistent = libxlDomainIsPersistent, /* 0.9.0 */
> .domainIsUpdated = libxlDomainIsUpdated, /* 0.9.0 */
> + .domainGetJobInfo = libxlDomainGetJobInfo, /* 0.10.0 */
> + .domainAbortJob = libxlDomainAbortJob, /* 0.10.0 */
> .domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */
> .domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */
> .isAlive = libxlIsAlive, /* 0.9.8 */
>
12:45 p.m.
New subject: [libvirt] [PATCH 2/2] Add lock in libxl api
Add long-running jobs for save, dump. Add normal job for the
api maybe modify the domain.
Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
---
src/libxl/libxl_driver.c | 219 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 158 insertions(+), 61 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d01d915..28e50b0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1534,9 +1534,13 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
goto cleanup;
def = NULL;
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0,
-1) < 0) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
goto cleanup;
}
@@ -1545,6 +1549,8 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (dom)
dom->id = vm->def->id;
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
virDomainDefFree(def);
if (vm)
@@ -1651,9 +1657,13 @@ libxlDomainSuspend(virDomainPtr dom)
_("No domain with matching uuid '%s'"),
uuidstr);
goto cleanup;
}
+
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
running"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -1663,7 +1673,7 @@ libxlDomainSuspend(virDomainPtr dom)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to suspend domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER);
@@ -1673,10 +1683,14 @@ libxlDomainSuspend(virDomainPtr dom)
}
if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
- goto cleanup;
+ goto endjob;
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1710,9 +1724,12 @@ libxlDomainResume(virDomainPtr dom)
goto cleanup;
}
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
running"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -1722,7 +1739,7 @@ libxlDomainResume(virDomainPtr dom)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to resume domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
@@ -1733,10 +1750,14 @@ libxlDomainResume(virDomainPtr dom)
}
if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
- goto cleanup;
+ goto endjob;
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1768,10 +1789,13 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -1779,7 +1803,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to shutdown domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
/* vm is marked shutoff (or removed from domains list if not persistent)
@@ -1787,6 +1811,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
*/
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1821,10 +1849,13 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -1832,10 +1863,14 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to reboot domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1864,10 +1899,13 @@ libxlDomainDestroyFlags(virDomainPtr dom,
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_DESTROY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
event = virDomainEventNewFromObj(vm,VIR_DOMAIN_EVENT_STOPPED,
@@ -1876,16 +1914,21 @@ libxlDomainDestroyFlags(virDomainPtr dom,
if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
dom->id);
- goto cleanup;
+ goto endjob;
}
if (!vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
ret = 0;
+endjob:
+ if ( vm && !libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1975,6 +2018,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
goto cleanup;
}
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
isActive = virDomainObjIsActive(vm);
if (flags == VIR_DOMAIN_MEM_CURRENT) {
@@ -1993,17 +2039,17 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot set memory on an inactive domain"));
- goto cleanup;
+ goto endjob;
}
if (flags & VIR_DOMAIN_MEM_CONFIG) {
if (!vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot change persistent config of a transient
domain"));
- goto cleanup;
+ goto endjob;
}
if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
- goto cleanup;
+ goto endjob;
}
if (flags & VIR_DOMAIN_MEM_MAXIMUM) {
@@ -2015,7 +2061,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set maximum memory for domain
'%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
}
@@ -2026,7 +2072,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
if (persistentDef->mem.cur_balloon > newmem)
persistentDef->mem.cur_balloon = newmem;
ret = virDomainSaveConfig(driver->configDir, persistentDef);
- goto cleanup;
+ goto endjob;
}
} else {
@@ -2035,7 +2081,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
if (newmem > vm->def->mem.max_balloon) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("cannot set memory higher than max memory"));
- goto cleanup;
+ goto endjob;
}
if (flags & VIR_DOMAIN_MEM_LIVE) {
@@ -2045,7 +2091,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set memory for domain
'%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
}
@@ -2053,11 +2099,14 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
sa_assert(persistentDef);
persistentDef->mem.cur_balloon = newmem;
ret = virDomainSaveConfig(driver->configDir, persistentDef);
- goto cleanup;
+ goto endjob;
}
}
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
if (vm)
@@ -2165,22 +2214,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
int fd;
int ret = -1;
+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm,
+ LIBXL_ASYNC_JOB_SAVE) < 0)
+ goto cleanup;
+
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
virReportError(VIR_ERR_OPERATION_INVALID,
_("Domain '%d' has to be running because libxenlight
will"
" suspend it"), vm->def->id);
- goto cleanup;
+ goto endjob;
}
if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR,
-1, -1, 0)) < 0) {
virReportSystemError(-fd,
_("Failed to create domain save file
'%s'"), to);
- goto cleanup;
+ goto endjob;
}
if ((xml = virDomainDefFormat(vm->def, 0)) == NULL)
- goto cleanup;
+ goto endjob;
xml_len = strlen(xml) + 1;
memset(&hdr, 0, sizeof(hdr));
@@ -2191,20 +2244,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
if (safewrite(fd, &hdr, sizeof(hdr)) != sizeof(hdr)) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to write save file header"));
- goto cleanup;
+ goto endjob;
}
if (safewrite(fd, xml, xml_len) != xml_len) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to write xml description"));
- goto cleanup;
+ goto endjob;
}
- if (libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd) != 0) {
+ virDomainObjUnlock(vm);
+ libxlDriverUnlock(driver);
+ ret = libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd);
+ libxlDriverLock(driver);
+ virDomainObjLock(vm);
+
+ if (ret != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to save domain '%d' with
libxenlight"),
vm->def->id);
- goto cleanup;
+ goto endjob;
}
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
@@ -2213,18 +2272,23 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_SAVED) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
vm->def->id);
- goto cleanup;
+ goto endjob;
}
vm->hasManagedSave = true;
if (!vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndAsyncJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
ret = 0;
+endjob:
+ if ( vm && !libxlDomainObjEndAsyncJob(driver, vm))
+ vm = NULL;
+
cleanup:
VIR_FREE(xml);
if (VIR_CLOSE(fd) < 0)
@@ -2312,12 +2376,18 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
def = NULL;
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if ((ret = libxlVmStart(driver, vm, false, fd)) < 0 &&
!vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndAsyncJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
+ if (vm && !libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
if (VIR_CLOSE(fd) < 0)
virReportSystemError(errno, "%s", _("cannot close file"));
@@ -2348,7 +2418,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int
flags)
libxlDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
- libxlDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2358,9 +2427,13 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int
flags)
goto cleanup;
}
+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm,
+ LIBXL_ASYNC_JOB_DUMP) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
running"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -2372,25 +2445,29 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int
flags)
_("Before dumping core, failed to suspend domain
'%d'"
" with libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP);
paused = true;
}
- if (libxl_domain_core_dump(&priv->ctx, dom->id, to) != 0) {
+ virDomainObjUnlock(vm);
+ libxlDriverUnlock(driver);
+ ret = libxl_domain_core_dump(&priv->ctx, dom->id, to);
+ libxlDriverLock(driver);
+ virDomainObjLock(vm);
+ if (ret != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to dump core of domain '%d' with
libxenlight"),
dom->id);
- goto cleanup_unpause;
+ goto endjob_unpause;
}
- libxlDriverLock(driver);
if (flags & VIR_DUMP_CRASH) {
if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
dom->id);
- goto cleanup_unlock;
+ goto endjob_unpause;
}
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
@@ -2398,15 +2475,14 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int
flags)
}
if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndAsyncJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
ret = 0;
-cleanup_unlock:
- libxlDriverUnlock(driver);
-cleanup_unpause:
+endjob_unpause:
if (virDomainObjIsActive(vm) && paused) {
if (libxl_domain_unpause(&priv->ctx, dom->id) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2417,14 +2493,15 @@ cleanup_unpause:
VIR_DOMAIN_RUNNING_UNPAUSED);
}
}
+endjob:
+ if (vm && !libxlDomainObjEndAsyncJob(driver, vm))
+ vm = NULL;
cleanup:
if (vm)
virDomainObjUnlock(vm);
- if (event) {
- libxlDriverLock(driver);
+ if (event)
libxlDomainEventQueue(driver, event);
- libxlDriverUnlock(driver);
- }
+ libxlDriverUnlock(driver);
return ret;
}
@@ -2601,22 +2678,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
goto cleanup;
}
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_VCPU_LIVE)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot set vcpus on an inactive domain"));
- goto cleanup;
+ goto endjob;
}
if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot change persistent config of a transient
domain"));
- goto cleanup;
+ goto endjob;
}
if ((max = libxlGetMaxVcpus(dom->conn, NULL)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("could not determine max vcpus for the domain"));
- goto cleanup;
+ goto endjob;
}
if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus <
max) {
@@ -2627,18 +2707,18 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virReportError(VIR_ERR_INVALID_ARG,
_("requested vcpus is greater than max allowable"
" vcpus for the domain: %d > %d"), nvcpus, max);
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
if (!(def = virDomainObjGetPersistentDef(driver->caps, vm)))
- goto cleanup;
+ goto endjob;
maplen = VIR_CPU_MAPLEN(nvcpus);
if (VIR_ALLOC_N(bitmask, maplen) < 0) {
virReportOOMError();
- goto cleanup;
+ goto endjob;
}
for (i = 0; i < nvcpus; ++i) {
@@ -2665,7 +2745,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set vcpus for domain '%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
break;
@@ -2674,7 +2754,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set vcpus for domain '%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
def->vcpus = nvcpus;
break;
@@ -2685,6 +2765,9 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
if (flags & VIR_DOMAIN_VCPU_CONFIG)
ret = virDomainSaveConfig(driver->configDir, def);
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
VIR_FREE(bitmask);
if (vm)
@@ -3053,14 +3136,21 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is already running"));
- goto cleanup;
+ goto endjob;
}
ret = libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1);
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -3589,6 +3679,9 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (virDomainObjIsActive(vm)) {
if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
@@ -3599,14 +3692,14 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
}
if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot modify device on transient
domain"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -3614,11 +3707,11 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
VIR_DOMAIN_XML_INACTIVE)))
- goto cleanup;
+ goto endjob;
/* Make a copy for updated domain. */
if (!(vmdef = virDomainObjCopyPersistentDef(driver->caps, vm)))
- goto cleanup;
+ goto endjob;
switch (action) {
case LIBXL_DEVICE_ATTACH:
@@ -3642,7 +3735,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
virDomainDeviceDefFree(dev);
if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
VIR_DOMAIN_XML_INACTIVE)))
- goto cleanup;
+ goto endjob;
switch (action) {
case LIBXL_DEVICE_ATTACH:
@@ -3675,6 +3768,10 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
}
}
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
virDomainDefFree(vmdef);
virDomainDeviceDefFree(dev);
--
1.7.12
6:10 p.m.
New subject: [libvirt] [PATCH 2/2] Add lock in libxl api
Bamvor Jian Zhang wrote:
Add long-running jobs for save, dump. Add normal job for the
api maybe modify the domain.
Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
---
src/libxl/libxl_driver.c | 219 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 158 insertions(+), 61 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d01d915..28e50b0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1534,9 +1534,13 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
goto cleanup;
def = NULL;
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0,
-1) < 0) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
goto cleanup;
}
@@ -1545,6 +1549,8 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
if (dom)
dom->id = vm->def->id;
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
virDomainDefFree(def);
if (vm)
@@ -1651,9 +1657,13 @@ libxlDomainSuspend(virDomainPtr dom)
_("No domain with matching uuid '%s'"),
uuidstr);
goto cleanup;
}
+
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
running"));
- goto cleanup;
+ goto endjob;
}
IMO, we should check if the domain is active before calling
libxlDomainObjBeginJob().
priv = vm->privateData;
@@ -1663,7 +1673,7 @@ libxlDomainSuspend(virDomainPtr dom)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to suspend domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER);
@@ -1673,10 +1683,14 @@ libxlDomainSuspend(virDomainPtr dom)
}
if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
- goto cleanup;
+ goto endjob;
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1710,9 +1724,12 @@ libxlDomainResume(virDomainPtr dom)
goto cleanup;
}
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
running"));
- goto cleanup;
+ goto endjob;
}
Same here.
priv = vm->privateData;
@@ -1722,7 +1739,7 @@ libxlDomainResume(virDomainPtr dom)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to resume domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
@@ -1733,10 +1750,14 @@ libxlDomainResume(virDomainPtr dom)
}
if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
- goto cleanup;
+ goto endjob;
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1768,10 +1789,13 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
And here.
priv = vm->privateData;
@@ -1779,7 +1803,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to shutdown domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
/* vm is marked shutoff (or removed from domains list if not persistent)
@@ -1787,6 +1811,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
*/
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1821,10 +1849,13 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
And here.
priv = vm->privateData;
@@ -1832,10 +1863,14 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to reboot domain '%d' with
libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1864,10 +1899,13 @@ libxlDomainDestroyFlags(virDomainPtr dom,
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_DESTROY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
I think you get the picture :).
event = virDomainEventNewFromObj(vm,VIR_DOMAIN_EVENT_STOPPED,
@@ -1876,16 +1914,21 @@ libxlDomainDestroyFlags(virDomainPtr dom,
if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
dom->id);
- goto cleanup;
+ goto endjob;
}
if (!vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
ret = 0;
+endjob:
+ if ( vm && !libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -1975,6 +2018,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
goto cleanup;
}
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
isActive = virDomainObjIsActive(vm);
if (flags == VIR_DOMAIN_MEM_CURRENT) {
@@ -1993,17 +2039,17 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long
newmem,
if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot set memory on an inactive domain"));
- goto cleanup;
+ goto endjob;
}
if (flags & VIR_DOMAIN_MEM_CONFIG) {
if (!vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot change persistent config of a transient
domain"));
- goto cleanup;
+ goto endjob;
}
if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
- goto cleanup;
+ goto endjob;
}
if (flags & VIR_DOMAIN_MEM_MAXIMUM) {
@@ -2015,7 +2061,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set maximum memory for domain
'%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
}
@@ -2026,7 +2072,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
if (persistentDef->mem.cur_balloon > newmem)
persistentDef->mem.cur_balloon = newmem;
ret = virDomainSaveConfig(driver->configDir, persistentDef);
- goto cleanup;
+ goto endjob;
}
} else {
@@ -2035,7 +2081,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
if (newmem > vm->def->mem.max_balloon) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("cannot set memory higher than max memory"));
- goto cleanup;
+ goto endjob;
}
if (flags & VIR_DOMAIN_MEM_LIVE) {
@@ -2045,7 +2091,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set memory for domain
'%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
}
@@ -2053,11 +2099,14 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long
newmem,
sa_assert(persistentDef);
persistentDef->mem.cur_balloon = newmem;
ret = virDomainSaveConfig(driver->configDir, persistentDef);
- goto cleanup;
+ goto endjob;
}
}
ret = 0;
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
if (vm)
@@ -2165,22 +2214,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
int fd;
int ret = -1;
+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm,
+ LIBXL_ASYNC_JOB_SAVE) < 0)
+ goto cleanup;
+
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
virReportError(VIR_ERR_OPERATION_INVALID,
_("Domain '%d' has to be running because libxenlight
will"
" suspend it"), vm->def->id);
- goto cleanup;
+ goto endjob;
}
if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR,
-1, -1, 0)) < 0) {
virReportSystemError(-fd,
_("Failed to create domain save file
'%s'"), to);
- goto cleanup;
+ goto endjob;
}
if ((xml = virDomainDefFormat(vm->def, 0)) == NULL)
- goto cleanup;
+ goto endjob;
xml_len = strlen(xml) + 1;
memset(&hdr, 0, sizeof(hdr));
@@ -2191,20 +2244,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
if (safewrite(fd, &hdr, sizeof(hdr)) != sizeof(hdr)) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to write save file header"));
- goto cleanup;
+ goto endjob;
}
if (safewrite(fd, xml, xml_len) != xml_len) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to write xml description"));
- goto cleanup;
+ goto endjob;
}
- if (libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd) != 0) {
+ virDomainObjUnlock(vm);
+ libxlDriverUnlock(driver);
+ ret = libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd);
+ libxlDriverLock(driver);
+ virDomainObjLock(vm);
+
+ if (ret != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to save domain '%d' with
libxenlight"),
vm->def->id);
- goto cleanup;
+ goto endjob;
}
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
@@ -2213,18 +2272,23 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_SAVED) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
vm->def->id);
- goto cleanup;
+ goto endjob;
}
vm->hasManagedSave = true;
if (!vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndAsyncJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
ret = 0;
+endjob:
+ if ( vm && !libxlDomainObjEndAsyncJob(driver, vm))
+ vm = NULL;
+
cleanup:
VIR_FREE(xml);
if (VIR_CLOSE(fd) < 0)
@@ -2312,12 +2376,18 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
def = NULL;
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
From my testing, this prevents listing domains, getting domain info,
etc. while restoring the domain. As mentioned in patch 1, we could use
LIBXL_ASYNC_JOB_RESTORE here.
+
if ((ret = libxlVmStart(driver, vm, false, fd)) < 0 &&
!vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndAsyncJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
Shouldn't we call virDomainRemoveInactive() even if
libxlDomainObjEndAsyncJob() fails?
vm = NULL;
}
+ if (vm && !libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
if (VIR_CLOSE(fd) < 0)
virReportSystemError(errno, "%s", _("cannot close file"));
@@ -2348,7 +2418,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int
flags)
libxlDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
- libxlDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2358,9 +2427,13 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int
flags)
goto cleanup;
}
+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm,
+ LIBXL_ASYNC_JOB_DUMP) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not
running"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -2372,25 +2445,29 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned
int flags)
_("Before dumping core, failed to suspend domain
'%d'"
" with libxenlight"),
dom->id);
- goto cleanup;
+ goto endjob;
}
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP);
paused = true;
}
- if (libxl_domain_core_dump(&priv->ctx, dom->id, to) != 0) {
+ virDomainObjUnlock(vm);
+ libxlDriverUnlock(driver);
+ ret = libxl_domain_core_dump(&priv->ctx, dom->id, to);
+ libxlDriverLock(driver);
+ virDomainObjLock(vm);
+ if (ret != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to dump core of domain '%d' with
libxenlight"),
dom->id);
- goto cleanup_unpause;
+ goto endjob_unpause;
}
- libxlDriverLock(driver);
if (flags & VIR_DUMP_CRASH) {
if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
dom->id);
- goto cleanup_unlock;
+ goto endjob_unpause;
}
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
@@ -2398,15 +2475,14 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned
int flags)
}
if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
- virDomainRemoveInactive(&driver->domains, vm);
+ if (libxlDomainObjEndAsyncJob(driver, vm))
+ virDomainRemoveInactive(&driver->domains, vm);
Same comment here about conditionally calling virDomainRemoveInactive().
Regards,
Jim
vm = NULL;
}
ret = 0;
-cleanup_unlock:
- libxlDriverUnlock(driver);
-cleanup_unpause:
+endjob_unpause:
if (virDomainObjIsActive(vm) && paused) {
if (libxl_domain_unpause(&priv->ctx, dom->id) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2417,14 +2493,15 @@ cleanup_unpause:
VIR_DOMAIN_RUNNING_UNPAUSED);
}
}
+endjob:
+ if (vm && !libxlDomainObjEndAsyncJob(driver, vm))
+ vm = NULL;
cleanup:
if (vm)
virDomainObjUnlock(vm);
- if (event) {
- libxlDriverLock(driver);
+ if (event)
libxlDomainEventQueue(driver, event);
- libxlDriverUnlock(driver);
- }
+ libxlDriverUnlock(driver);
return ret;
}
@@ -2601,22 +2678,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
goto cleanup;
}
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_VCPU_LIVE)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot set vcpus on an inactive domain"));
- goto cleanup;
+ goto endjob;
}
if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot change persistent config of a transient
domain"));
- goto cleanup;
+ goto endjob;
}
if ((max = libxlGetMaxVcpus(dom->conn, NULL)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("could not determine max vcpus for the domain"));
- goto cleanup;
+ goto endjob;
}
if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus <
max) {
@@ -2627,18 +2707,18 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virReportError(VIR_ERR_INVALID_ARG,
_("requested vcpus is greater than max allowable"
" vcpus for the domain: %d > %d"), nvcpus, max);
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
if (!(def = virDomainObjGetPersistentDef(driver->caps, vm)))
- goto cleanup;
+ goto endjob;
maplen = VIR_CPU_MAPLEN(nvcpus);
if (VIR_ALLOC_N(bitmask, maplen) < 0) {
virReportOOMError();
- goto cleanup;
+ goto endjob;
}
for (i = 0; i < nvcpus; ++i) {
@@ -2665,7 +2745,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set vcpus for domain '%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
break;
@@ -2674,7 +2754,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set vcpus for domain '%d'"
" with libxenlight"), dom->id);
- goto cleanup;
+ goto endjob;
}
def->vcpus = nvcpus;
break;
@@ -2685,6 +2765,9 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus,
if (flags & VIR_DOMAIN_VCPU_CONFIG)
ret = virDomainSaveConfig(driver->configDir, def);
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
VIR_FREE(bitmask);
if (vm)
@@ -3053,14 +3136,21 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is already running"));
- goto cleanup;
+ goto endjob;
}
ret = libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1);
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virDomainObjUnlock(vm);
@@ -3589,6 +3679,9 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
goto cleanup;
}
+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (virDomainObjIsActive(vm)) {
if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
@@ -3599,14 +3692,14 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("Domain is not running"));
- goto cleanup;
+ goto endjob;
}
}
if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot modify device on transient
domain"));
- goto cleanup;
+ goto endjob;
}
priv = vm->privateData;
@@ -3614,11 +3707,11 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
VIR_DOMAIN_XML_INACTIVE)))
- goto cleanup;
+ goto endjob;
/* Make a copy for updated domain. */
if (!(vmdef = virDomainObjCopyPersistentDef(driver->caps, vm)))
- goto cleanup;
+ goto endjob;
switch (action) {
case LIBXL_DEVICE_ATTACH:
@@ -3642,7 +3735,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
virDomainDeviceDefFree(dev);
if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
VIR_DOMAIN_XML_INACTIVE)))
- goto cleanup;
+ goto endjob;
switch (action) {
case LIBXL_DEVICE_ATTACH:
@@ -3675,6 +3768,10 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
}
}
+endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
virDomainDefFree(vmdef);
virDomainDeviceDefFree(dev);
11:06 a.m.
New subject: [libvirt] [PATCH 2/2] Add lock in libxl api
On 10/22/2012 05:10 PM, Jim Fehlig wrote:
Bamvor Jian Zhang wrote:
> Add long-running jobs for save, dump. Add normal job for the
> api maybe modify the domain.
>
> Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
> +
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is
not running"));
> - goto cleanup;
> + goto endjob;
> }
>
IMO, we should check if the domain is active before calling
libxlDomainObjBeginJob().
That's a possible optimization to avoid contending for the lock, but the
point remains that libxlDomainObjBeginJob() temporarily drops locks, and
while locks are down, the domain can stop. Therefore, you must ALWAYS
check if the domain is active after obtaining the job, even if you
already checked prior to requesting the job. This particular section of
code looks correct to me as-is.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:01 p.m.
New subject: [libvirt] [PATCH 2/2] Add lock in libxl api
Eric Blake wrote:
On 10/22/2012 05:10 PM, Jim Fehlig wrote:
> Bamvor Jian Zhang wrote:
>
>> Add long-running jobs for save, dump. Add normal job for the
>> api maybe modify the domain.
>>
>> Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
>>
>> +
>> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>> + goto cleanup;
>> +
>> if (!virDomainObjIsActive(vm)) {
>> virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain
is not running"));
>> - goto cleanup;
>> + goto endjob;
>> }
>>
>>
> IMO, we should check if the domain is active before calling
> libxlDomainObjBeginJob().
>
That's a possible optimization to avoid contending for the lock, but the
point remains that libxlDomainObjBeginJob() temporarily drops locks, and
while locks are down, the domain can stop.
Oh, right. Thanks for catching that.
Therefore, you must ALWAYS
check if the domain is active after obtaining the job, even if you
already checked prior to requesting the job. This particular section of
code looks correct to me as-is.
Agreed. Bamvor, ignore my comments about changing this pattern
throughout the patch.
Regards,
Jim
3:35 a.m.
New subject: [libvirt] [PATCH 2/2] Add lock in libxl api
>>Jim Fehlig <jfehlig(a)suse.com> wrote:
Bamvor Jian Zhang wrote:
> Add long-running jobs for save, dump. Add normal job for the
> api maybe modify the domain.
>
> Signed-off-by: Bamvor Jian Zhang <bjzhang(a)suse.com>
> ---
> src/libxl/libxl_driver.c | 219 ++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 158 insertions(+), 61 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d01d915..28e50b0 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1534,9 +1534,13 @@ libxlDomainCreateXML(virConnectPtr conn, const char
*xml,
> goto cleanup;
> def = NULL;
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0,
> -1) < 0) {
> - virDomainRemoveInactive(&driver->domains, vm);
> + if (libxlDomainObjEndJob(driver, vm))
> + virDomainRemoveInactive(&driver->domains, vm);
> vm = NULL;
> goto cleanup;
> }
> @@ -1545,6 +1549,8 @@ libxlDomainCreateXML(virConnectPtr conn, const char
*xml,
> if (dom)
> dom->id = vm->def->id;
>
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> cleanup:
> virDomainDefFree(def);
> if (vm)
> @@ -1651,9 +1657,13 @@ libxlDomainSuspend(virDomainPtr dom)
> _("No domain with matching uuid '%s'"),
uuidstr);
> goto cleanup;
> }
> +
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is
not
running"));
> - goto cleanup;
> + goto endjob;
> }
>
IMO, we should check if the domain is active before calling
libxlDomainObjBeginJob().
>
> priv = vm->privateData;
> @@ -1663,7 +1673,7 @@ libxlDomainSuspend(virDomainPtr dom)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to suspend domain '%d' with
libxenlight"),
> dom->id);
> - goto cleanup;
> + goto endjob;
> }
>
> virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
VIR_DOMAIN_PAUSED_USER);
> @@ -1673,10 +1683,14 @@ libxlDomainSuspend(virDomainPtr dom)
> }
>
> if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
> - goto cleanup;
> + goto endjob;
>
> ret = 0;
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> @@ -1710,9 +1724,12 @@ libxlDomainResume(virDomainPtr dom)
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is
not
running"));
> - goto cleanup;
> + goto endjob;
> }
>
Same here.
>
> priv = vm->privateData;
> @@ -1722,7 +1739,7 @@ libxlDomainResume(virDomainPtr dom)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to resume domain '%d' with
libxenlight"),
> dom->id);
> - goto cleanup;
> + goto endjob;
> }
>
> virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
> @@ -1733,10 +1750,14 @@ libxlDomainResume(virDomainPtr dom)
> }
>
> if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
> - goto cleanup;
> + goto endjob;
>
> ret = 0;
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> @@ -1768,10 +1789,13 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned
int flags)
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("Domain is not running"));
> - goto cleanup;
> + goto endjob;
> }
>
And here.
>
> priv = vm->privateData;
> @@ -1779,7 +1803,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned
int flags)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to shutdown domain '%d' with
libxenlight"),
> dom->id);
> - goto cleanup;
> + goto endjob;
> }
>
> /* vm is marked shutoff (or removed from domains list if not
persistent)
> @@ -1787,6 +1811,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned
int flags)
> */
> ret = 0;
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> @@ -1821,10 +1849,13 @@ libxlDomainReboot(virDomainPtr dom, unsigned int
flags)
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("Domain is not running"));
> - goto cleanup;
> + goto endjob;
> }
>
And here.
>
> priv = vm->privateData;
> @@ -1832,10 +1863,14 @@ libxlDomainReboot(virDomainPtr dom, unsigned int
flags)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to reboot domain '%d' with
libxenlight"),
> dom->id);
> - goto cleanup;
> + goto endjob;
> }
> ret = 0;
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> @@ -1864,10 +1899,13 @@ libxlDomainDestroyFlags(virDomainPtr dom,
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_DESTROY) <
0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("Domain is not running"));
> - goto cleanup;
> + goto endjob;
> }
>
I think you get the picture :).
>
> event = virDomainEventNewFromObj(vm,VIR_DOMAIN_EVENT_STOPPED,
> @@ -1876,16 +1914,21 @@ libxlDomainDestroyFlags(virDomainPtr dom,
> if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to destroy domain '%d'"),
dom->id);
> - goto cleanup;
> + goto endjob;
> }
>
> if (!vm->persistent) {
> - virDomainRemoveInactive(&driver->domains, vm);
> + if (libxlDomainObjEndJob(driver, vm))
> + virDomainRemoveInactive(&driver->domains, vm);
> vm = NULL;
> }
>
> ret = 0;
>
> +endjob:
> + if ( vm && !libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> @@ -1975,6 +2018,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> isActive = virDomainObjIsActive(vm);
>
> if (flags == VIR_DOMAIN_MEM_CURRENT) {
> @@ -1993,17 +2039,17 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot set memory on an inactive domain"));
> - goto cleanup;
> + goto endjob;
> }
>
> if (flags & VIR_DOMAIN_MEM_CONFIG) {
> if (!vm->persistent) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot change persistent config of a
transient domain"));
> - goto cleanup;
> + goto endjob;
> }
> if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps,
vm)))
> - goto cleanup;
> + goto endjob;
> }
>
> if (flags & VIR_DOMAIN_MEM_MAXIMUM) {
> @@ -2015,7 +2061,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to set maximum memory for domain
'%d'"
> " with libxenlight"), dom->id);
> - goto cleanup;
> + goto endjob;
> }
> }
>
> @@ -2026,7 +2072,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> if (persistentDef->mem.cur_balloon > newmem)
> persistentDef->mem.cur_balloon = newmem;
> ret = virDomainSaveConfig(driver->configDir, persistentDef);
> - goto cleanup;
> + goto endjob;
> }
>
> } else {
> @@ -2035,7 +2081,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> if (newmem > vm->def->mem.max_balloon) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("cannot set memory higher than max
memory"));
> - goto cleanup;
> + goto endjob;
> }
>
> if (flags & VIR_DOMAIN_MEM_LIVE) {
> @@ -2045,7 +2091,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to set memory for domain
'%d'"
> " with libxenlight"), dom->id);
> - goto cleanup;
> + goto endjob;
> }
> }
>
> @@ -2053,11 +2099,14 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned
long newmem,
> sa_assert(persistentDef);
> persistentDef->mem.cur_balloon = newmem;
> ret = virDomainSaveConfig(driver->configDir, persistentDef);
> - goto cleanup;
> + goto endjob;
> }
> }
>
> ret = 0;
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
>
> cleanup:
> if (vm)
> @@ -2165,22 +2214,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
> int fd;
> int ret = -1;
>
> + if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm,
> + LIBXL_ASYNC_JOB_SAVE) < 0)
> + goto cleanup;
> +
> if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> _("Domain '%d' has to be running because
libxenlight will"
> " suspend it"), vm->def->id);
> - goto cleanup;
> + goto endjob;
> }
>
> if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR,
> -1, -1, 0)) < 0) {
> virReportSystemError(-fd,
> _("Failed to create domain save file
'%s'"),
to);
> - goto cleanup;
> + goto endjob;
> }
>
> if ((xml = virDomainDefFormat(vm->def, 0)) == NULL)
> - goto cleanup;
> + goto endjob;
> xml_len = strlen(xml) + 1;
>
> memset(&hdr, 0, sizeof(hdr));
> @@ -2191,20 +2244,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
> if (safewrite(fd, &hdr, sizeof(hdr)) != sizeof(hdr)) {
> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> _("Failed to write save file header"));
> - goto cleanup;
> + goto endjob;
> }
>
> if (safewrite(fd, xml, xml_len) != xml_len) {
> virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> _("Failed to write xml description"));
> - goto cleanup;
> + goto endjob;
> }
>
> - if (libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd) != 0)
{
> + virDomainObjUnlock(vm);
> + libxlDriverUnlock(driver);
> + ret = libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd);
> + libxlDriverLock(driver);
> + virDomainObjLock(vm);
> +
> + if (ret != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to save domain '%d' with
libxenlight"),
> vm->def->id);
> - goto cleanup;
> + goto endjob;
> }
>
> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> @@ -2213,18 +2272,23 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
> if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_SAVED) != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to destroy domain '%d'"),
vm->def->id);
> - goto cleanup;
> + goto endjob;
> }
>
> vm->hasManagedSave = true;
>
> if (!vm->persistent) {
> - virDomainRemoveInactive(&driver->domains, vm);
> + if (libxlDomainObjEndAsyncJob(driver, vm))
> + virDomainRemoveInactive(&driver->domains, vm);
> vm = NULL;
> }
>
> ret = 0;
>
> +endjob:
> + if ( vm && !libxlDomainObjEndAsyncJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> VIR_FREE(xml);
> if (VIR_CLOSE(fd) < 0)
> @@ -2312,12 +2376,18 @@ libxlDomainRestoreFlags(virConnectPtr conn, const
char *from,
>
> def = NULL;
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
>
From my testing, this prevents listing domains, getting domain info,
etc. while restoring the domain. As mentioned in patch 1, we could use
LIBXL_ASYNC_JOB_RESTORE here.
> +
> if ((ret = libxlVmStart(driver, vm, false, fd)) < 0 &&
> !vm->persistent) {
> - virDomainRemoveInactive(&driver->domains, vm);
> + if (libxlDomainObjEndAsyncJob(driver, vm))
> + virDomainRemoveInactive(&driver->domains, vm);
>
Shouldn't we call virDomainRemoveInactive() even if
libxlDomainObjEndAsyncJob() fails?
virDomainRemoveInactive depends on
virDomainObjPtr. the failure
of libxlDomainObjEndJob or libxlDomainObjEndAsyncJob lead to
virDomainObjPtr non-existed.
meanwhile libxlDomainObjEndxxxJob should not failed while it is paired
with libxlDomainObjBeginxxxJob.
So, i though it is using the code like this. virDomainRemoveInactive will
be called definitely.
> vm = NULL;
> }
>
> + if (vm && !libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> cleanup:
> if (VIR_CLOSE(fd) < 0)
> virReportSystemError(errno, "%s", _("cannot close
file"));
> @@ -2348,7 +2418,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to,
unsigned int flags)
>
> libxlDriverLock(driver);
> vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> - libxlDriverUnlock(driver);
>
> if (!vm) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -2358,9 +2427,13 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to,
unsigned int flags)
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm,
> + LIBXL_ASYNC_JOB_DUMP) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is
not
running"));
> - goto cleanup;
> + goto endjob;
> }
>
> priv = vm->privateData;
> @@ -2372,25 +2445,29 @@ libxlDomainCoreDump(virDomainPtr dom, const char
*to, unsigned int flags)
> _("Before dumping core, failed to suspend
domain '%d'"
> " with libxenlight"),
> dom->id);
> - goto cleanup;
> + goto endjob;
> }
> virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
VIR_DOMAIN_PAUSED_DUMP);
> paused = true;
> }
>
> - if (libxl_domain_core_dump(&priv->ctx, dom->id, to) != 0) {
> + virDomainObjUnlock(vm);
> + libxlDriverUnlock(driver);
> + ret = libxl_domain_core_dump(&priv->ctx, dom->id, to);
> + libxlDriverLock(driver);
> + virDomainObjLock(vm);
> + if (ret != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to dump core of domain '%d' with
libxenlight"),
> dom->id);
> - goto cleanup_unpause;
> + goto endjob_unpause;
> }
>
> - libxlDriverLock(driver);
> if (flags & VIR_DUMP_CRASH) {
> if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to destroy domain '%d'"),
dom->id);
> - goto cleanup_unlock;
> + goto endjob_unpause;
> }
>
> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> @@ -2398,15 +2475,14 @@ libxlDomainCoreDump(virDomainPtr dom, const char
*to, unsigned int flags)
> }
>
> if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
> - virDomainRemoveInactive(&driver->domains, vm);
> + if (libxlDomainObjEndAsyncJob(driver, vm))
> + virDomainRemoveInactive(&driver->domains, vm);
>
Same comment here about conditionally calling virDomainRemoveInactive().
Regards,
Jim
> vm = NULL;
> }
>
> ret = 0;
>
> -cleanup_unlock:
> - libxlDriverUnlock(driver);
> -cleanup_unpause:
> +endjob_unpause:
> if (virDomainObjIsActive(vm) && paused) {
> if (libxl_domain_unpause(&priv->ctx, dom->id) != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2417,14 +2493,15 @@ cleanup_unpause:
> VIR_DOMAIN_RUNNING_UNPAUSED);
> }
> }
> +endjob:
> + if (vm && !libxlDomainObjEndAsyncJob(driver, vm))
> + vm = NULL;
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> - if (event) {
> - libxlDriverLock(driver);
> + if (event)
> libxlDomainEventQueue(driver, event);
> - libxlDriverUnlock(driver);
> - }
> + libxlDriverUnlock(driver);
> return ret;
> }
>
> @@ -2601,22 +2678,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned
int nvcpus,
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_VCPU_LIVE)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot set vcpus on an inactive domain"));
> - goto cleanup;
> + goto endjob;
> }
>
> if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot change persistent config of a transient
domain"));
> - goto cleanup;
> + goto endjob;
> }
>
> if ((max = libxlGetMaxVcpus(dom->conn, NULL)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("could not determine max vcpus for the
domain"));
> - goto cleanup;
> + goto endjob;
> }
>
> if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus
< max) {
> @@ -2627,18 +2707,18 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned
int nvcpus,
> virReportError(VIR_ERR_INVALID_ARG,
> _("requested vcpus is greater than max allowable"
> " vcpus for the domain: %d > %d"), nvcpus,
max);
> - goto cleanup;
> + goto endjob;
> }
>
> priv = vm->privateData;
>
> if (!(def = virDomainObjGetPersistentDef(driver->caps, vm)))
> - goto cleanup;
> + goto endjob;
>
> maplen = VIR_CPU_MAPLEN(nvcpus);
> if (VIR_ALLOC_N(bitmask, maplen) < 0) {
> virReportOOMError();
> - goto cleanup;
> + goto endjob;
> }
>
> for (i = 0; i < nvcpus; ++i) {
> @@ -2665,7 +2745,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned
int nvcpus,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to set vcpus for domain
'%d'"
> " with libxenlight"), dom->id);
> - goto cleanup;
> + goto endjob;
> }
> break;
>
> @@ -2674,7 +2754,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned
int nvcpus,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to set vcpus for domain
'%d'"
> " with libxenlight"), dom->id);
> - goto cleanup;
> + goto endjob;
> }
> def->vcpus = nvcpus;
> break;
> @@ -2685,6 +2765,9 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned
int nvcpus,
> if (flags & VIR_DOMAIN_VCPU_CONFIG)
> ret = virDomainSaveConfig(driver->configDir, def);
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> cleanup:
> VIR_FREE(bitmask);
> if (vm)
> @@ -3053,14 +3136,21 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("Domain is already running"));
> - goto cleanup;
> + goto endjob;
> }
>
> ret = libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0,
-1);
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> if (vm)
> virDomainObjUnlock(vm);
> @@ -3589,6 +3679,9 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const
char *xml,
> goto cleanup;
> }
>
> + if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> if (virDomainObjIsActive(vm)) {
> if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)
> flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
> @@ -3599,14 +3692,14 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const
char *xml,
> if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("Domain is not running"));
> - goto cleanup;
> + goto endjob;
> }
> }
>
> if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) &&
!vm->persistent) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("cannot modify device on transient
domain"));
> - goto cleanup;
> + goto endjob;
> }
>
> priv = vm->privateData;
> @@ -3614,11 +3707,11 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const
char *xml,
> if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
> VIR_DOMAIN_XML_INACTIVE)))
> - goto cleanup;
> + goto endjob;
>
> /* Make a copy for updated domain. */
> if (!(vmdef = virDomainObjCopyPersistentDef(driver->caps, vm)))
> - goto cleanup;
> + goto endjob;
>
> switch (action) {
> case LIBXL_DEVICE_ATTACH:
> @@ -3642,7 +3735,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const
char *xml,
> virDomainDeviceDefFree(dev);
> if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml,
> VIR_DOMAIN_XML_INACTIVE)))
> - goto cleanup;
> + goto endjob;
>
> switch (action) {
> case LIBXL_DEVICE_ATTACH:
> @@ -3675,6 +3768,10 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const
char *xml,
> }
> }
>
> +endjob:
> + if (!libxlDomainObjEndJob(driver, vm))
> + vm = NULL;
> +
> cleanup:
> virDomainDefFree(vmdef);
> virDomainDeviceDefFree(dev);
>
4404
days inactive
4426
days old
9 comments
4 participants
participants (4)
-
Bamvor Jian Zhang -
Daniel P. Berrange -
Eric Blake -
Jim Fehlig