8:46 a.m.
The following series of 5 patches implement basic thread safety for the
QEMU, LXC and Network drivers. It does not address the OpenVZ or Test
driver yet. The Xen driver is totally stateless so does not require
changes - though I do need to verify there's no 'static' variables that
are used in an unsafe yet in Xen drivers.
Also note that these patches are *not* bisectable at this time, because
the domain config APIs add locking to some generic APIs, and unless the
drivers using these APIs are updated at same time deadlocks will occur.
I need to re-arrange ordering of changes a little to make it bisectable.
The core idea here is to have 2 levels of locking
- A per-driver lock.
- A per-virDomainObjPtr/NetworkObjPtr lock
For every API call, the driver lock is initialy taken. If the API involves
use of a domain / network object, the 2nd object specific lock is taken,
and the driver lock can (optionally) be released at this time.
When creating / deleting domain/network objects though, the driver lock
has to be held for the entire duration of the API call. Fortunately these
API calls don't have very long critical sections.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:49 a.m.
New subject: [libvirt] PATCH: 1/5: Don't pass driver object to QEMU monitor API
We don't want to hold a lock on the driver object for the duration of
each monitor call, and indeed we should not even need to use it. So
this patch stops passing the driver object to the QEMU monitor API
We're basically changing
static int qemudMonitorCommand (const struct qemud_driver *driver,
const virDomainObjPtr vm,
const char *cmd,
char **reply);
To:
static int qemudMonitorCommand (const virDomainObjPtr vm,
const char *cmd,
char **reply);
This arg was already redundant and annotated ATTRIBUTE_UNUSED so no
functional change here
Regards,
Daniel
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -118,8 +118,7 @@ static void qemudShutdownVMDaemon(virCon
static int qemudDomainGetMaxVcpus(virDomainPtr dom);
-static int qemudMonitorCommand (const struct qemud_driver *driver,
- const virDomainObjPtr vm,
+static int qemudMonitorCommand (const virDomainObjPtr vm,
const char *cmd,
char **reply);
@@ -311,14 +310,12 @@ qemudShutdown(void) {
/* Return -1 for error, 1 to continue reading and 0 for success */
typedef int qemudHandlerMonitorOutput(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm,
const char *output,
int fd);
static int
qemudReadMonitorOutput(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm,
int fd,
char *buf,
@@ -377,7 +374,7 @@ qemudReadMonitorOutput(virConnectPtr con
} else {
got += ret;
buf[got] = '\0';
- if ((ret = func(conn, driver, vm, buf, fd)) != 1)
+ if ((ret = func(conn, vm, buf, fd)) != 1)
return ret;
}
}
@@ -391,7 +388,6 @@ qemudReadMonitorOutput(virConnectPtr con
static int
qemudCheckMonitorPrompt(virConnectPtr conn ATTRIBUTE_UNUSED,
- struct qemud_driver *driver ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
const char *output,
int fd)
@@ -405,7 +401,6 @@ qemudCheckMonitorPrompt(virConnectPtr co
}
static int qemudOpenMonitor(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm,
const char *monitor) {
int monfd;
@@ -429,7 +424,7 @@ static int qemudOpenMonitor(virConnectPt
}
ret = qemudReadMonitorOutput(conn,
- driver, vm, monfd,
+ vm, monfd,
buf, sizeof(buf),
qemudCheckMonitorPrompt,
"monitor");
@@ -489,7 +484,6 @@ static int qemudExtractMonitorPath(virCo
static int
qemudFindCharDevicePTYs(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm,
const char *output,
int fd ATTRIBUTE_UNUSED)
@@ -528,7 +522,7 @@ qemudFindCharDevicePTYs(virConnectPtr co
}
/* Got them all, so now open the monitor console */
- ret = qemudOpenMonitor(conn, driver, vm, monitor);
+ ret = qemudOpenMonitor(conn, vm, monitor);
cleanup:
VIR_FREE(monitor);
@@ -536,11 +530,10 @@ cleanup:
}
static int qemudWaitForMonitor(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm) {
char buf[1024]; /* Plenty of space to get startup greeting */
int ret = qemudReadMonitorOutput(conn,
- driver, vm, vm->stderr_fd,
+ vm, vm->stderr_fd,
buf, sizeof(buf),
qemudFindCharDevicePTYs,
"console");
@@ -557,7 +550,6 @@ static int qemudWaitForMonitor(virConnec
static int
qemudDetectVcpuPIDs(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm) {
char *qemucpus = NULL;
char *line;
@@ -581,7 +573,7 @@ qemudDetectVcpuPIDs(virConnectPtr conn,
return 0;
}
- if (qemudMonitorCommand(driver, vm, "info cpus", &qemucpus) < 0) {
+ if (qemudMonitorCommand(vm, "info cpus", &qemucpus) < 0) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot run monitor command to fetch CPU
thread info"));
VIR_FREE(vm->vcpupids);
@@ -657,7 +649,6 @@ error:
static int
qemudInitCpus(virConnectPtr conn,
- struct qemud_driver *driver,
virDomainObjPtr vm) {
char *info = NULL;
#if HAVE_SCHED_GETAFFINITY
@@ -695,7 +686,7 @@ qemudInitCpus(virConnectPtr conn,
#endif /* HAVE_SCHED_GETAFFINITY */
/* Allow the CPUS to start executing */
- if (qemudMonitorCommand(driver, vm, "cont", &info) < 0) {
+ if (qemudMonitorCommand(vm, "cont", &info) < 0) {
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("resume operation failed"));
return -1;
@@ -912,9 +903,9 @@ static int qemudStartVMDaemon(virConnect
POLLIN | POLLERR | POLLHUP,
qemudDispatchVMEvent,
driver) < 0) ||
- (qemudWaitForMonitor(conn, driver, vm) < 0) ||
- (qemudDetectVcpuPIDs(conn, driver, vm) < 0) ||
- (qemudInitCpus(conn, driver, vm) < 0)) {
+ (qemudWaitForMonitor(conn, vm) < 0) ||
+ (qemudDetectVcpuPIDs(conn, vm) < 0) ||
+ (qemudInitCpus(conn, vm) < 0)) {
qemudShutdownVMDaemon(conn, driver, vm);
return -1;
}
@@ -1043,8 +1034,7 @@ static void qemudDispatchVMEvent(int fd,
}
static int
-qemudMonitorCommand (const struct qemud_driver *driver ATTRIBUTE_UNUSED,
- const virDomainObjPtr vm,
+qemudMonitorCommand (const virDomainObjPtr vm,
const char *cmd,
char **reply) {
int size = 0;
@@ -1496,7 +1486,7 @@ static int qemudDomainSuspend(virDomainP
if (vm->state == VIR_DOMAIN_PAUSED)
return 0;
- if (qemudMonitorCommand(driver, vm, "stop", &info) < 0) {
+ if (qemudMonitorCommand(vm, "stop", &info) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("suspend operation failed"));
return -1;
@@ -1524,7 +1514,7 @@ static int qemudDomainResume(virDomainPt
}
if (vm->state == VIR_DOMAIN_RUNNING)
return 0;
- if (qemudMonitorCommand(driver, vm, "cont", &info) < 0) {
+ if (qemudMonitorCommand(vm, "cont", &info) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("resume operation failed"));
return -1;
@@ -1547,7 +1537,7 @@ static int qemudDomainShutdown(virDomain
return -1;
}
- if (qemudMonitorCommand(driver, vm, "system_powerdown", &info) < 0)
{
+ if (qemudMonitorCommand(vm, "system_powerdown", &info) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("shutdown operation failed"));
return -1;
@@ -1875,7 +1865,7 @@ static int qemudDomainSave(virDomainPtr
}
free(safe_path);
- if (qemudMonitorCommand(driver, vm, command, &info) < 0) {
+ if (qemudMonitorCommand(vm, command, &info) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("migrate operation failed"));
VIR_FREE(command);
@@ -2201,7 +2191,7 @@ static int qemudDomainRestore(virConnect
/* If it was running before, resume it now. */
if (header.was_running) {
char *info;
- if (qemudMonitorCommand(driver, vm, "cont", &info) < 0) {
+ if (qemudMonitorCommand(vm, "cont", &info) < 0) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to resume domain"));
return -1;
@@ -2481,7 +2471,7 @@ static int qemudDomainChangeEjectableMed
}
VIR_FREE(devname);
- if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
+ if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("cannot change cdrom media"));
VIR_FREE(cmd);
@@ -2552,7 +2542,7 @@ static int qemudDomainAttachDiskDevice(v
return ret;
}
- if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
+ if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
_("cannot attach %s disk"), type);
VIR_FREE(cmd);
@@ -2603,7 +2593,7 @@ static int qemudDomainAttachUsbMassstora
return ret;
}
- if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
+ if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("cannot attach usb disk"));
VIR_FREE(cmd);
@@ -2662,7 +2652,7 @@ static int qemudDomainAttachHostDevice(v
return -1;
}
- if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
+ if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("cannot attach usb device"));
VIR_FREE(cmd);
@@ -2875,7 +2865,7 @@ qemudDomainBlockStats (virDomainPtr dom,
return -1;
len = strlen (qemu_dev_name);
- if (qemudMonitorCommand (driver, vm, "info blockstats", &info) < 0)
{
+ if (qemudMonitorCommand (vm, "info blockstats", &info) < 0) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("'info blockstats' command
failed"));
goto out;
@@ -3107,7 +3097,7 @@ qemudDomainMemoryPeek (virDomainPtr dom,
/* Issue the memsave command. */
snprintf (cmd, sizeof cmd, "memsave %llu %zi \"%s\"", offset,
size, tmp);
- if (qemudMonitorCommand (driver, vm, cmd, &info) < 0) {
+ if (qemudMonitorCommand (vm, cmd, &info) < 0) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("'memsave' command
failed"));
goto done;
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:57 a.m.
New subject: [libvirt] PATCH: 2/5: Locking on virDomainObjPtr objects
This patch provides for locking of the virDomainObjPtr instances on a per
object level, and defines locking semantics of the various API calls
that use these objects.
Since this locking only covers the individual objects, it is expected that
the driver provide a higher level of locking around arrays of objects.
I considering putting an explicit lock on the virDomainObjListPtr array,
but this adds more complexity in the drivers, and does not improve
safety or concurrency.
* Domain lookup methods
virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms,
int id);
virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
const unsigned char *uuid);
virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
const char *name);
The driver must hold its global lock to protect the virDomainObjListPtr
object. The returned virDomainObjPtr instance will be locked at which
point the driver can optionall release its global lock
* Creating virDomainObjPtr instances
virDomainObjPtr virDomainAssignDef(virConnectPtr conn,
virDomainObjListPtr doms,
const virDomainDefPtr def);
The driver must hold its global lock to protect the virDomainObjListPtr
object. The newly created or cached virDomainObjPtr instance that is
returned will be locked.
* Removing a virDomainObjPtr instsance:
void virDomainRemoveInactive(virDomainObjListPtr doms,
virDomainObjPtr dom);
The driver must hold its global lock to protect the virDomaiNObjListPtr
object. The virDomainObjPtr must be unlocked.
For all the other methods in domain_conf.h, which deal with the config
in the virDomainDefPtr instances, the caller must hold a lock over the
virDomainObjPtr instance which owns the virDomainDefPtr.
This may all sound fairly onerous / complex, but it is straightforward
to work with in the drivers, as the following patches will demonstrate
Daniel
diff --git a/src/domain_conf.c b/src/domain_conf.c
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -145,40 +145,70 @@ VIR_ENUM_IMPL(virDomainHostdevSubsys, VI
__virReportErrorHelper(conn, VIR_FROM_DOMAIN, code, __FILE__, \
__FUNCTION__, __LINE__, fmt)
+/**
+ * @virDomainFindByID:
+ *
+ * Caller must hold exclusive lock over 'doms'
+ *
+ * Returns a virDomainObjPtr locked for exclusive access, or NULL
+ */
virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms,
int id)
{
unsigned int i;
- for (i = 0 ; i < doms->count ; i++)
+ for (i = 0 ; i < doms->count ; i++) {
+ virDomainLock(doms->objs[i]);
if (virDomainIsActive(doms->objs[i]) &&
doms->objs[i]->def->id == id)
return doms->objs[i];
+ virDomainUnlock(doms->objs[i]);
+ }
return NULL;
}
+/**
+ * @virDomainFindByUUID:
+ *
+ * Caller must hold exclusive lock over 'doms'
+ *
+ * Returns a virDomainObjPtr locked for exclusive access, or NULL
+ */
virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
const unsigned char *uuid)
{
unsigned int i;
- for (i = 0 ; i < doms->count ; i++)
+ for (i = 0 ; i < doms->count ; i++) {
+ virDomainLock(doms->objs[i]);
if (!memcmp(doms->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN))
return doms->objs[i];
+ virDomainUnlock(doms->objs[i]);
+ }
return NULL;
}
+/**
+ * @virDomainFindByName:
+ *
+ * Caller must hold exclusive lock over 'doms'
+ *
+ * Returns a virDomainObjPtr locked for exclusive access, or NULL
+ */
virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
const char *name)
{
unsigned int i;
- for (i = 0 ; i < doms->count ; i++)
+ for (i = 0 ; i < doms->count ; i++) {
+ virDomainLock(doms->objs[i]);
if (STREQ(doms->objs[i]->def->name, name))
return doms->objs[i];
+ virDomainUnlock(doms->objs[i]);
+ }
return NULL;
}
@@ -406,6 +436,12 @@ void virDomainDefFree(virDomainDefPtr de
VIR_FREE(def);
}
+/**
+ * @virDomainObjListFree
+ *
+ * Caller should not hold lock on 'dom', but must
+ * also ensure no other thread can have an active lock
+ */
void virDomainObjFree(virDomainObjPtr dom)
{
if (!dom)
@@ -419,17 +455,35 @@ void virDomainObjFree(virDomainObjPtr do
VIR_FREE(dom);
}
-void virDomainObjListFree(virDomainObjListPtr vms)
+/**
+ * @virDomainObjListFree
+ *
+ * Caller must hold exclusive lock over 'doms'
+ */
+void virDomainObjListFree(virDomainObjListPtr doms)
{
unsigned int i;
- for (i = 0 ; i < vms->count ; i++)
- virDomainObjFree(vms->objs[i]);
+ for (i = 0 ; i < doms->count ; i++) {
+ /* Lock+unlock ensures no one is still using this dom */
+ virDomainLock(doms->objs[i]);
+ virDomainUnlock(doms->objs[i]);
- VIR_FREE(vms->objs);
- vms->count = 0;
+ virDomainObjFree(doms->objs[i]);
+ }
+
+ VIR_FREE(doms->objs);
+ doms->count = 0;
}
+
+/**
+ * @virDomainAssignDef
+ *
+ * Caller must hold exclusive lock over 'doms'.
+ *
+ * The return domain object will be locked
+ */
virDomainObjPtr virDomainAssignDef(virConnectPtr conn,
virDomainObjListPtr doms,
const virDomainDefPtr def)
@@ -456,6 +510,7 @@ virDomainObjPtr virDomainAssignDef(virCo
domain->state = VIR_DOMAIN_SHUTOFF;
domain->def = def;
+ pthread_mutex_init(&domain->lock, NULL);
if (VIR_REALLOC_N(doms->objs, doms->count + 1) < 0) {
virDomainReportError(conn, VIR_ERR_NO_MEMORY, NULL);
@@ -466,13 +521,26 @@ virDomainObjPtr virDomainAssignDef(virCo
doms->objs[doms->count] = domain;
doms->count++;
+ virDomainLock(domain);
+
return domain;
}
+
+/**
+ * @virDomainRemoveInactive:
+ *
+ * Caller must hold exclusive lock over 'doms', but
+ * 'dom' must be unlocked
+ */
void virDomainRemoveInactive(virDomainObjListPtr doms,
virDomainObjPtr dom)
{
unsigned int i;
+
+ /* Ensure no other thread holds a lock on dom */
+ virDomainLock(dom);
+ virDomainUnlock(dom);
for (i = 0 ; i < doms->count ; i++) {
if (doms->objs[i] == dom) {
@@ -3335,8 +3403,10 @@ int virDomainLoadAllConfigs(virConnectPt
configDir,
autostartDir,
entry->d_name);
- if (dom)
+ if (dom) {
dom->persistent = 1;
+ virDomainUnlock(dom);
+ }
}
closedir(dir);
diff --git a/src/domain_conf.h b/src/domain_conf.h
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -452,6 +452,8 @@ typedef struct _virDomainObj virDomainOb
typedef struct _virDomainObj virDomainObj;
typedef virDomainObj *virDomainObjPtr;
struct _virDomainObj {
+ pthread_mutex_t lock;
+
int stdin_fd;
int stdout_fd;
int stderr_fd;
@@ -481,6 +483,20 @@ virDomainIsActive(virDomainObjPtr dom)
virDomainIsActive(virDomainObjPtr dom)
{
return dom->def->id != -1;
+}
+
+
+static inline int
+virDomainLock(virDomainObjPtr dom)
+{
+ return pthread_mutex_lock(&dom->lock);
+}
+
+
+static inline int
+virDomainUnlock(virDomainObjPtr dom)
+{
+ return pthread_mutex_unlock(&dom->lock);
}
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:03 a.m.
New subject: [libvirt] PATCH: 3/5: Locking in the QEMU driver
This patch makes use of the virDomainObjPtr locking infrastructure to
provide thread safety in the QEMU driver. The QEMU driver itself gains
a global lock. The idea is this lock is only held for short periods of
time - typically only while obtaining a virDomainObjPtr instance.
The main limitation of the current locking implemented here, is that
the virDomainObjPtr lock is held for the duration of all monitor
commands. Some of these can take quite a long time to execute, so for
a useful level of concurrency we need to drop the lock & reobtain it
when issuing monitor commands. I intend to deal with this problem in
an add-on patch, since I'd rather keep the initial impl simple to
understand.
The general model for an API call involving a virDomainPtr is
qemudXXXXX(virDomainPtr dom) {
struct qemud_driver *driver = conn->privateData;
virDomainObjPtr vm;
qemudDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, uuid);
qemudDriverUnlock(driver);
.... do work ....
virDomainObjUnlock(vm);
return 0;
error:
virDomainObjUnlock(vm);
return -1;
}
The important thing to note is that if anything goes wrong in the
'... do work...' bit of the driver call, it should never do
a direct 'return -1;'. It must always 'goto error' instead. This
avoids the need to sprinkle calls to virDomainObjUnlock(vm)
everywhere.
Daniel
diff --git a/src/qemu_conf.h b/src/qemu_conf.h
--- a/src/qemu_conf.h
+++ b/src/qemu_conf.h
@@ -48,6 +48,8 @@ enum qemud_cmd_flags {
/* Main driver state */
struct qemud_driver {
+ pthread_mutex_t lock;
+
unsigned int qemuVersion;
int nextvmid;
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -76,6 +76,22 @@ static int qemudShutdown(void);
#define qemudLog(level, msg...) fprintf(stderr, msg)
+
+static int
+qemudDriverLock(struct qemud_driver *driver)
+{
+ DEBUG("LOCK driver %p %p", (driver), (void*)pthread_self());
+ return pthread_mutex_lock(&(driver)->lock);
+}
+
+static int
+qemudDriverUnlock(struct qemud_driver *driver)
+{
+ DEBUG("UNLOCK driver %p %p", (driver), (void*)pthread_self());
+ return pthread_mutex_unlock(&(driver)->lock);
+}
+
+
static int qemudSetCloseExec(int fd) {
int flags;
if ((flags = fcntl(fd, F_GETFD)) < 0)
@@ -130,13 +146,18 @@ qemudAutostartConfigs(struct qemud_drive
unsigned int i;
for (i = 0 ; i < driver->domains.count ; i++) {
- if (driver->domains.objs[i]->autostart &&
- !virDomainIsActive(driver->domains.objs[i]) &&
- qemudStartVMDaemon(NULL, driver, driver->domains.objs[i], NULL) < 0) {
+ virDomainObjPtr dom = driver->domains.objs[i];
+ virDomainLock(dom);
+
+ if (dom->autostart &&
+ !virDomainIsActive(dom) &&
+ qemudStartVMDaemon(NULL, driver, dom, NULL) < 0) {
virErrorPtr err = virGetLastError();
qemudLog(QEMUD_ERR, _("Failed to autostart VM '%s':
%s\n"),
- driver->domains.objs[i]->def->name, err->message);
+ dom->def->name, err->message);
}
+
+ virDomainUnlock(dom);
}
}
@@ -157,6 +178,7 @@ qemudStartup(void) {
/* Don't have a dom0 so start from 1 */
qemu_driver->nextvmid = 1;
+ pthread_mutex_init(&qemu_driver->lock, NULL);
if (!uid) {
if (asprintf(&qemu_driver->logDir,
@@ -203,15 +225,20 @@ qemudStartup(void) {
return -1;
}
+ qemudDriverLock(qemu_driver);
+
if (virDomainLoadAllConfigs(NULL,
qemu_driver->caps,
&qemu_driver->domains,
qemu_driver->configDir,
qemu_driver->autostartDir) < 0) {
+ qemudDriverUnlock(qemu_driver);
qemudShutdown();
return -1;
}
qemudAutostartConfigs(qemu_driver);
+
+ qemudDriverUnlock(qemu_driver);
return 0;
@@ -235,6 +262,8 @@ qemudReload(void) {
if (!qemu_driver)
return 0;
+ qemudDriverLock(qemu_driver);
+
virDomainLoadAllConfigs(NULL,
qemu_driver->caps,
&qemu_driver->domains,
@@ -242,6 +271,8 @@ qemudReload(void) {
qemu_driver->autostartDir);
qemudAutostartConfigs(qemu_driver);
+
+ qemudDriverUnlock(qemu_driver);
return 0;
}
@@ -257,16 +288,23 @@ static int
static int
qemudActive(void) {
unsigned int i;
+ int isactive = 0;
if (!qemu_driver)
return 0;
- for (i = 0 ; i < qemu_driver->domains.count ; i++)
+ qemudDriverLock(qemu_driver);
+
+ for (i = 0 ; i < qemu_driver->domains.count ; i++) {
+ virDomainLock(qemu_driver->domains.objs[i]);
if (virDomainIsActive(qemu_driver->domains.objs[i]))
- return 1;
+ isactive = 1;
+ virDomainUnlock(qemu_driver->domains.objs[i]);
+ }
- /* Otherwise we're happy to deal with a shutdown */
- return 0;
+ qemudDriverUnlock(qemu_driver);
+
+ return isactive;
}
/**
@@ -1222,12 +1260,16 @@ static char *qemudGetCapabilities(virCon
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
char *xml;
+ qemudDriverLock(driver);
+
if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) {
+ qemudDriverUnlock(driver);
qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY,
"%s", _("failed to allocate space for capabilities
support"));
return NULL;
}
+ qemudDriverUnlock(driver);
return xml;
}
@@ -1327,8 +1369,12 @@ static virDomainPtr qemudDomainLookupByI
static virDomainPtr qemudDomainLookupByID(virConnectPtr conn,
int id) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, id);
+ virDomainObjPtr vm;
virDomainPtr dom;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, id);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -1337,13 +1383,19 @@ static virDomainPtr qemudDomainLookupByI
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
+
+ virDomainUnlock(vm);
return dom;
}
static virDomainPtr qemudDomainLookupByUUID(virConnectPtr conn,
const unsigned char *uuid) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, uuid);
+ virDomainObjPtr vm;
virDomainPtr dom;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -1352,13 +1404,19 @@ static virDomainPtr qemudDomainLookupByU
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
+
+ virDomainUnlock(vm);
return dom;
}
static virDomainPtr qemudDomainLookupByName(virConnectPtr conn,
const char *name) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
- virDomainObjPtr vm = virDomainFindByName(&driver->domains, name);
+ virDomainObjPtr vm;
virDomainPtr dom;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByName(&driver->domains, name);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -1367,15 +1425,23 @@ static virDomainPtr qemudDomainLookupByN
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
+
+ virDomainUnlock(vm);
return dom;
}
static int qemudGetVersion(virConnectPtr conn, unsigned long *version) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
- if (qemudExtractVersion(conn, driver) < 0)
+ qemudDriverLock(driver);
+
+ if (qemudExtractVersion(conn, driver) < 0) {
+ qemudDriverUnlock(driver);
return -1;
+ }
*version = qemu_driver->qemuVersion;
+
+ qemudDriverUnlock(driver);
return 0;
}
@@ -1405,39 +1471,51 @@ static int qemudListDomains(virConnectPt
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
int got = 0, i;
- for (i = 0 ; i < driver->domains.count && got < nids ; i++)
+ qemudDriverLock(driver);
+ for (i = 0 ; i < driver->domains.count && got < nids ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (virDomainIsActive(driver->domains.objs[i]))
ids[got++] = driver->domains.objs[i]->def->id;
+ virDomainUnlock(driver->domains.objs[i]);
+ }
+ qemudDriverUnlock(driver);
return got;
}
+
static int qemudNumDomains(virConnectPtr conn) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
int n = 0, i;
- for (i = 0 ; i < driver->domains.count ; i++)
+ qemudDriverLock(driver);
+ for (i = 0 ; i < driver->domains.count ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (virDomainIsActive(driver->domains.objs[i]))
n++;
+ virDomainUnlock(driver->domains.objs[i]);
+ }
+ qemudDriverUnlock(driver);
return n;
}
static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
unsigned int flags ATTRIBUTE_UNUSED) {
virDomainDefPtr def;
- virDomainObjPtr vm;
+ virDomainObjPtr vm = NULL;
virDomainPtr dom;
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
+ qemudDriverLock(driver);
+
if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
- return NULL;
+ goto cleanup;
vm = virDomainFindByName(&driver->domains, def->name);
if (vm) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("domain '%s' is already defined"),
def->name);
- virDomainDefFree(def);
- return NULL;
+ goto cleanup;
}
vm = virDomainFindByUUID(&driver->domains, def->uuid);
if (vm) {
@@ -1447,149 +1525,225 @@ static virDomainPtr qemudDomainCreate(vi
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("domain with uuid '%s' is already
defined"),
uuidstr);
- virDomainDefFree(def);
- return NULL;
+ goto cleanup;
}
if (!(vm = virDomainAssignDef(conn,
&driver->domains,
- def))) {
- virDomainDefFree(def);
- return NULL;
- }
+ def)))
+ goto cleanup;
- if (qemudStartVMDaemon(conn, driver, vm, NULL) < 0) {
- virDomainRemoveInactive(&driver->domains,
- vm);
- return NULL;
- }
+ /* XXX should figure out how to release driver lock here
+ * so we don't block during startup */
+ if (qemudStartVMDaemon(conn, driver, vm, NULL) < 0)
+ goto cleanup;
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
+
+ virDomainUnlock(vm);
+ qemudDriverUnlock(driver);
return dom;
+
+cleanup:
+ if (vm) {
+ virDomainUnlock(vm);
+ virDomainRemoveInactive(&driver->domains,
+ vm);
+ } else {
+ virDomainDefFree(def);
+ }
+
+ qemudDriverUnlock(driver);
+ return NULL;
}
static int qemudDomainSuspend(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- char *info;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ char *info = NULL;
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
+
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, _("no
domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("domain is not running"));
- return -1;
+ goto cleanup;
}
- if (vm->state == VIR_DOMAIN_PAUSED)
- return 0;
- if (qemudMonitorCommand(vm, "stop", &info) < 0) {
- qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
- "%s", _("suspend operation failed"));
- return -1;
+ if (vm->state != VIR_DOMAIN_PAUSED) {
+ if (qemudMonitorCommand(vm, "stop", &info) < 0) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("suspend operation failed"));
+ goto cleanup;
+ }
+ vm->state = VIR_DOMAIN_PAUSED;
+ qemudDebug("Reply %s", info);
+ VIR_FREE(info);
}
- vm->state = VIR_DOMAIN_PAUSED;
- qemudDebug("Reply %s", info);
+
+ virDomainUnlock(vm);
+ return 0;
+
+cleanup:
VIR_FREE(info);
- return 0;
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainResume(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- char *info;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ char *info = NULL;
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
+
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("domain is not running"));
- return -1;
+ goto cleanup;
}
- if (vm->state == VIR_DOMAIN_RUNNING)
- return 0;
- if (qemudMonitorCommand(vm, "cont", &info) < 0) {
- qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
- "%s", _("resume operation failed"));
- return -1;
+ if (vm->state != VIR_DOMAIN_RUNNING) {
+ if (qemudMonitorCommand(vm, "cont", &info) < 0) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+ "%s", _("resume operation failed"));
+ goto cleanup;
+ }
+ vm->state = VIR_DOMAIN_RUNNING;
+ qemudDebug("Reply %s", info);
+ VIR_FREE(info);
}
- vm->state = VIR_DOMAIN_RUNNING;
- qemudDebug("Reply %s", info);
+
+ virDomainUnlock(vm);
+ return 0;
+
+cleanup:
VIR_FREE(info);
- return 0;
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainShutdown(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
- char* info;
+ virDomainObjPtr vm;
+ char* info = NULL;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
if (qemudMonitorCommand(vm, "system_powerdown", &info) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("shutdown operation failed"));
- return -1;
+ goto cleanup;
}
VIR_FREE(info);
+ virDomainUnlock(vm);
return 0;
+cleanup:
+ VIR_FREE(info);
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainDestroy(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
qemudShutdownVMDaemon(dom->conn, driver, vm);
- if (!vm->persistent)
+ if (!vm->persistent) {
+ virDomainUnlock(vm);
virDomainRemoveInactive(&driver->domains,
vm);
+ return 0;
+ }
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static char *qemudDomainGetOSType(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
char *type;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return NULL;
+ goto cleanup;
}
if (!(type = strdup(vm->def->os.type))) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY,
"%s", _("failed to allocate space for
ostype"));
- return NULL;
+ goto cleanup;
}
+
+ virDomainUnlock(vm);
return type;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return NULL;
}
/* Returns max memory in kb, 0 if error */
static unsigned long qemudDomainGetMaxMemory(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+ unsigned long ret;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1597,15 +1751,27 @@ static unsigned long qemudDomainGetMaxMe
virUUIDFormat(dom->uuid, uuidstr);
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching uuid '%s'"),
uuidstr);
- return 0;
+ goto cleanup;
}
- return vm->def->maxmem;
+ ret = vm->def->maxmem;
+ virDomainUnlock(vm);
+
+ return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return 0;
}
static int qemudDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1613,22 +1779,32 @@ static int qemudDomainSetMaxMemory(virDo
virUUIDFormat(dom->uuid, uuidstr);
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching uuid '%s'"),
uuidstr);
- return -1;
+ goto cleanup;
}
if (newmax < vm->def->memory) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s", _("cannot set max memory lower than current
memory"));
- return -1;
+ goto cleanup;
}
vm->def->maxmem = newmax;
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1636,33 +1812,44 @@ static int qemudDomainSetMemory(virDomai
virUUIDFormat(dom->uuid, uuidstr);
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching uuid '%s'"),
uuidstr);
- return -1;
+ goto cleanup;
}
if (virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("cannot set memory of an active
domain"));
- return -1;
+ goto cleanup;
}
if (newmem > vm->def->maxmem) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s", _("cannot set memory higher than max
memory"));
- return -1;
+ goto cleanup;
}
vm->def->memory = newmem;
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainGetInfo(virDomainPtr dom,
virDomainInfoPtr info) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
+
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return -1;
+ goto cleanup;
}
info->state = vm->state;
@@ -1671,15 +1858,23 @@ static int qemudDomainGetInfo(virDomainP
info->cpuTime = 0;
} else {
if (qemudGetProcessInfo(&(info->cpuTime), vm->pid) < 0) {
- qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
("cannot read cputime for domain"));
- return -1;
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+ _("cannot read cputime for domain"));
+ goto cleanup;
}
}
info->maxMem = vm->def->maxmem;
info->memory = vm->def->memory;
info->nrVirtCpu = vm->def->vcpus;
+
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
@@ -1779,12 +1974,16 @@ static int qemudDomainSave(virDomainPtr
static int qemudDomainSave(virDomainPtr dom,
const char *path) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ virDomainObjPtr vm;
char *command, *info;
- int fd;
+ int fd = -1;
char *safe_path;
- char *xml;
+ char *xml = NULL;
struct qemud_save_header header;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
memset(&header, 0, sizeof(header));
memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
@@ -1793,13 +1992,13 @@ static int qemudDomainSave(virDomainPtr
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("domain is not running"));
- return -1;
+ goto cleanup;
}
/* Pause */
@@ -1808,7 +2007,7 @@ static int qemudDomainSave(virDomainPtr
if (qemudDomainSuspend(dom) != 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to pause domain"));
- return -1;
+ goto cleanup;
}
}
@@ -1817,7 +2016,7 @@ static int qemudDomainSave(virDomainPtr
if (!xml) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to get domain xml"));
- return -1;
+ goto cleanup;
}
header.xml_len = strlen(xml) + 1;
@@ -1825,24 +2024,19 @@ static int qemudDomainSave(virDomainPtr
if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
_("failed to create '%s'"), path);
- VIR_FREE(xml);
- return -1;
+ goto cleanup;
}
if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to write save header"));
- close(fd);
- VIR_FREE(xml);
- return -1;
+ goto cleanup;
}
if (safewrite(fd, xml, header.xml_len) != header.xml_len) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to write xml"));
- close(fd);
- VIR_FREE(xml);
- return -1;
+ goto cleanup;
}
close(fd);
@@ -1890,18 +2084,33 @@ static int qemudDomainSave(virDomainPtr
/* Shut it down */
qemudShutdownVMDaemon(dom->conn, driver, vm);
- if (!vm->persistent)
+ if (!vm->persistent) {
+ virDomainUnlock(vm);
virDomainRemoveInactive(&driver->domains,
vm);
+ return 0;
+ }
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (fd != -1)
+ close(fd);
+ VIR_FREE(xml);
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
int max;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1909,30 +2118,37 @@ static int qemudDomainSetVcpus(virDomain
virUUIDFormat(dom->uuid, uuidstr);
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching uuid '%s'"),
uuidstr);
- return -1;
+ goto cleanup;
}
if (virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s",
_("cannot change vcpu count of an active domain"));
- return -1;
+ goto cleanup;
}
if ((max = qemudDomainGetMaxVcpus(dom)) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s",
_("could not determine max vcpus for the domain"));
- return -1;
+ goto cleanup;
}
if (nvcpus > max) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("requested vcpus is greater than max allowable"
" vcpus for the domain: %d > %d"), nvcpus, max);
- return -1;
+ goto cleanup;
}
vm->def->vcpus = nvcpus;
+
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
@@ -1943,26 +2159,39 @@ qemudDomainPinVcpu(virDomainPtr dom,
unsigned char *cpumap,
int maplen) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
cpu_set_t mask;
int i, maxcpu;
virNodeInfo nodeinfo;
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
+
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ virUUIDFormat(dom->uuid, uuidstr);
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s",_("cannot pin vcpus on an inactive
domain"));
- return -1;
+ goto cleanup;
}
if (vcpu > (vm->nvcpupids-1)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("vcpu number out of range %d > %d"),
vcpu, vm->nvcpupids);
- return -1;
+ goto cleanup;
}
if (virNodeInfoPopulate(dom->conn, &nodeinfo) < 0)
- return -1;
+ goto cleanup;
maxcpu = maplen * 8;
if (maxcpu > nodeinfo.cpus)
@@ -1978,15 +2207,21 @@ qemudDomainPinVcpu(virDomainPtr dom,
if (sched_setaffinity(vm->vcpupids[vcpu], sizeof(mask), &mask) < 0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("cannot set affinity: %s"), strerror(errno));
- return -1;
+ goto cleanup;
}
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("cpu affinity is not supported"));
- return -1;
+ goto cleanup;
}
+ virDomainUnlock(vm);
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int
@@ -1996,18 +2231,31 @@ qemudDomainGetVcpus(virDomainPtr dom,
unsigned char *cpumaps,
int maplen) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
virNodeInfo nodeinfo;
int i, v, maxcpu;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
+
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ virUUIDFormat(dom->uuid, uuidstr);
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s",_("cannot pin vcpus on an inactive
domain"));
- return -1;
+ goto cleanup;
}
if (virNodeInfoPopulate(dom->conn, &nodeinfo) < 0)
- return -1;
+ goto cleanup;
maxcpu = maplen * 8;
if (maxcpu > nodeinfo.cpus)
@@ -2017,8 +2265,10 @@ qemudDomainGetVcpus(virDomainPtr dom,
if (maxinfo > vm->nvcpupids)
maxinfo = vm->nvcpupids;
- if (maxinfo < 1)
+ if (maxinfo < 1) {
+ virDomainUnlock(vm);
return 0;
+ }
if (info != NULL) {
memset(info, 0, sizeof(*info) * maxinfo);
@@ -2040,7 +2290,7 @@ qemudDomainGetVcpus(virDomainPtr dom,
if (sched_getaffinity(vm->vcpupids[v], sizeof(mask), &mask) <
0) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("cannot get affinity: %s"),
strerror(errno));
- return -1;
+ goto cleanup;
}
for (i = 0 ; i < maxcpu ; i++)
@@ -2050,20 +2300,30 @@ qemudDomainGetVcpus(virDomainPtr dom,
} else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("cpu affinity is not
available"));
- return -1;
+ goto cleanup;
}
}
+ virDomainUnlock(vm);
return maxinfo;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
#endif /* HAVE_SCHED_GETAFFINITY */
static int qemudDomainGetMaxVcpus(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
const char *type;
int ret;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2071,21 +2331,27 @@ static int qemudDomainGetMaxVcpus(virDom
virUUIDFormat(dom->uuid, uuidstr);
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching uuid '%s'"),
uuidstr);
- return -1;
+ goto cleanup;
}
if (!(type = virDomainVirtTypeToString(vm->def->virtType))) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
_("unknown virt type in domain definition
'%d'"),
vm->def->virtType);
- return -1;
+ goto cleanup;
}
if ((ret = qemudGetMaxVCPUs(dom->conn, type)) < 0) {
- return -1;
+ goto cleanup;
}
+ virDomainUnlock(vm);
return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
@@ -2093,10 +2359,10 @@ static int qemudDomainRestore(virConnect
const char *path) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
virDomainDefPtr def;
- virDomainObjPtr vm;
- int fd;
+ virDomainObjPtr vm = NULL;
+ int fd = -1;
int ret;
- char *xml;
+ char *xml = NULL;
struct qemud_save_header header;
/* Verify the header and read the XML */
@@ -2143,13 +2409,14 @@ static int qemudDomainRestore(virConnect
return -1;
}
+
+ qemudDriverLock(driver);
+
/* Create a domain from this XML */
if (!(def = virDomainDefParseString(conn, driver->caps, xml))) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to parse XML"));
- close(fd);
- VIR_FREE(xml);
- return -1;
+ goto cleanup;
}
VIR_FREE(xml);
@@ -2160,9 +2427,10 @@ static int qemudDomainRestore(virConnect
if (vm && virDomainIsActive(vm)) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("domain is already active as '%s'"),
vm->def->name);
- close(fd);
- return -1;
+ goto cleanup;
}
+ if (vm)
+ virDomainUnlock(vm);
if (!(vm = virDomainAssignDef(conn,
&driver->domains,
@@ -2170,22 +2438,25 @@ static int qemudDomainRestore(virConnect
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to assign new VM"));
virDomainDefFree(def);
- close(fd);
- return -1;
+ goto cleanup;
}
/* Set the migration source and start it up. */
vm->stdin_fd = fd;
ret = qemudStartVMDaemon(conn, driver, vm, "stdio");
close(fd);
+ fd = -1;
vm->stdin_fd = -1;
if (ret < 0) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to start VM"));
- if (!vm->persistent)
+ if (!vm->persistent) {
+ virDomainUnlock(vm);
virDomainRemoveInactive(&driver->domains,
vm);
- return -1;
+ vm = NULL;
+ }
+ goto cleanup;
}
/* If it was running before, resume it now. */
@@ -2194,30 +2465,57 @@ static int qemudDomainRestore(virConnect
if (qemudMonitorCommand(vm, "cont", &info) < 0) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("failed to resume domain"));
- return -1;
+ goto cleanup;
}
VIR_FREE(info);
vm->state = VIR_DOMAIN_RUNNING;
}
+ virDomainUnlock(vm);
+ qemudDriverUnlock(driver);
return 0;
+
+cleanup:
+ if (fd != -1)
+ close(fd);
+ VIR_FREE(xml);
+ if (vm)
+ virDomainUnlock(vm);
+ qemudDriverUnlock(driver);
+ return -1;
}
static char *qemudDomainDumpXML(virDomainPtr dom,
int flags ATTRIBUTE_UNUSED) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+ char *ret;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
+
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return NULL;
+ goto cleanup;
}
- return virDomainDefFormat(dom->conn,
- (flags & VIR_DOMAIN_XML_INACTIVE) &&
vm->newDef ?
- vm->newDef : vm->def,
- flags);
+ ret = virDomainDefFormat(dom->conn,
+ (flags & VIR_DOMAIN_XML_INACTIVE) &&
vm->newDef ?
+ vm->newDef : vm->def,
+ flags);
+ if (!ret)
+ goto cleanup;
+
+ virDomainUnlock(vm);
+ return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return NULL;
}
@@ -2226,15 +2524,20 @@ static int qemudListDefinedDomains(virCo
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
int got = 0, i;
+ qemudDriverLock(driver);
for (i = 0 ; i < driver->domains.count && got < nnames ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (!virDomainIsActive(driver->domains.objs[i])) {
if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) {
qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY,
"%s", _("failed to allocate space for VM
name string"));
+ virDomainUnlock(driver->domains.objs[i]);
goto cleanup;
}
}
+ virDomainUnlock(driver->domains.objs[i]);
}
+ qemudDriverUnlock(driver);
return got;
@@ -2248,9 +2551,14 @@ static int qemudNumDefinedDomains(virCon
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
int n = 0, i;
- for (i = 0 ; i < driver->domains.count ; i++)
+ qemudDriverLock(driver);
+ for (i = 0 ; i < driver->domains.count ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (!virDomainIsActive(driver->domains.objs[i]))
n++;
+ virDomainUnlock(driver->domains.objs[i]);
+ }
+ qemudDriverUnlock(driver);
return n;
}
@@ -2258,77 +2566,110 @@ static int qemudNumDefinedDomains(virCon
static int qemudDomainStart(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+ int ret;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
+ qemudDriverUnlock(driver);
return -1;
}
- return qemudStartVMDaemon(dom->conn, driver, vm, NULL);
+ ret = qemudStartVMDaemon(dom->conn, driver, vm, NULL);
+ virDomainUnlock(vm);
+ qemudDriverUnlock(driver);
+ return ret;
}
static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) {
struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
virDomainDefPtr def;
- virDomainObjPtr vm;
+ virDomainObjPtr vm = NULL;
virDomainPtr dom;
+ qemudDriverLock(driver);
+
if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
- return NULL;
+ goto cleanup;
if (!(vm = virDomainAssignDef(conn,
&driver->domains,
def))) {
virDomainDefFree(def);
- return NULL;
+ goto cleanup;
}
vm->persistent = 1;
if (virDomainSaveConfig(conn,
driver->configDir,
vm->newDef ? vm->newDef : vm->def) < 0) {
+ virDomainUnlock(vm);
virDomainRemoveInactive(&driver->domains,
vm);
- return NULL;
+ vm = NULL;
+ goto cleanup;
}
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom) dom->id = vm->def->id;
+
+ virDomainUnlock(vm);
+ qemudDriverUnlock(driver);
return dom;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ qemudDriverUnlock(driver);
+ return NULL;
}
static int qemudDomainUndefine(virDomainPtr dom) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm = NULL;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return -1;
+ goto cleanup;
}
if (virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot delete active domain"));
- return -1;
+ goto cleanup;
}
if (!vm->persistent) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot undefine transient
domain"));
- return -1;
+ goto cleanup;
}
if (virDomainDeleteConfig(dom->conn, driver->configDir,
driver->autostartDir, vm) < 0)
- return -1;
+ goto cleanup;
+ virDomainUnlock(vm);
virDomainRemoveInactive(&driver->domains,
vm);
+ qemudDriverUnlock(driver);
+
return 0;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ qemudDriverLock(driver);
+ return -1;
}
/* Return the disks name for use in monitor commands */
@@ -2681,9 +3022,13 @@ static int qemudDomainAttachDevice(virDo
static int qemudDomainAttachDevice(virDomainPtr dom,
const char *xml) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
virDomainDeviceDefPtr dev;
int ret = 0, supported = 0;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -2694,13 +3039,11 @@ static int qemudDomainAttachDevice(virDo
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot attach device on inactive
domain"));
- return -1;
+ goto cleanup;
}
- dev = virDomainDeviceDefParse(dom->conn, vm->def, xml);
- if (dev == NULL) {
- return -1;
- }
+ if (!(dev = virDomainDeviceDefParse(dom->conn, vm->def, xml)))
+ goto cleanup;
if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
switch (dev->data.disk->device) {
@@ -2723,8 +3066,8 @@ static int qemudDomainAttachDevice(virDo
} else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
dev->data.hostdev->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
- supported = 1;
- ret = qemudDomainAttachHostDevice(dom, dev);
+ supported = 1;
+ ret = qemudDomainAttachHostDevice(dom, dev);
}
if (!supported) {
@@ -2734,13 +3077,24 @@ static int qemudDomainAttachDevice(virDo
}
VIR_FREE(dev);
+
+ virDomainUnlock(vm);
return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int qemudDomainGetAutostart(virDomainPtr dom,
int *autostart) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -2750,71 +3104,80 @@ static int qemudDomainGetAutostart(virDo
*autostart = vm->autostart;
+ virDomainUnlock(vm);
return 0;
}
static int qemudDomainSetAutostart(virDomainPtr dom,
int autostart) {
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
char *configFile = NULL, *autostartLink = NULL;
- int ret = -1;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return -1;
+ goto cleanup;
}
if (!vm->persistent) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot set autostart for transient
domain"));
- return -1;
+ goto cleanup;
}
autostart = (autostart != 0);
- if (vm->autostart == autostart)
- return 0;
+ if (vm->autostart != autostart) {
+ if ((configFile = virDomainConfigFile(dom->conn, driver->configDir,
vm->def->name)) == NULL)
+ goto cleanup;
+ if ((autostartLink = virDomainConfigFile(dom->conn, driver->autostartDir,
vm->def->name)) == NULL)
+ goto cleanup;
- if ((configFile = virDomainConfigFile(dom->conn, driver->configDir,
vm->def->name)) == NULL)
- goto cleanup;
- if ((autostartLink = virDomainConfigFile(dom->conn, driver->autostartDir,
vm->def->name)) == NULL)
- goto cleanup;
+ if (autostart) {
+ int err;
- if (autostart) {
- int err;
+ if ((err = virFileMakePath(driver->autostartDir))) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("cannot create autostart directory %s:
%s"),
+ driver->autostartDir, strerror(err));
+ goto cleanup;
+ }
- if ((err = virFileMakePath(driver->autostartDir))) {
- qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
- _("cannot create autostart directory %s: %s"),
- driver->autostartDir, strerror(err));
- goto cleanup;
+ if (symlink(configFile, autostartLink) < 0) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Failed to create symlink '%s to
'%s': %s"),
+ autostartLink, configFile, strerror(errno));
+ goto cleanup;
+ }
+ } else {
+ if (unlink(autostartLink) < 0 && errno != ENOENT && errno
!= ENOTDIR) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Failed to delete symlink '%s':
%s"),
+ autostartLink, strerror(errno));
+ goto cleanup;
+ }
}
- if (symlink(configFile, autostartLink) < 0) {
- qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
- _("Failed to create symlink '%s to '%s':
%s"),
- autostartLink, configFile, strerror(errno));
- goto cleanup;
- }
- } else {
- if (unlink(autostartLink) < 0 && errno != ENOENT && errno !=
ENOTDIR) {
- qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
- _("Failed to delete symlink '%s': %s"),
- autostartLink, strerror(errno));
- goto cleanup;
- }
+ vm->autostart = autostart;
}
- vm->autostart = autostart;
- ret = 0;
+ VIR_FREE(configFile);
+ VIR_FREE(autostartLink);
+ virDomainUnlock(vm);
+ return 0;
cleanup:
+ if (vm)
+ virDomainUnlock(vm);
VIR_FREE(configFile);
VIR_FREE(autostartLink);
- return ret;
+ return -1;
}
/* This uses the 'info blockstats' monitor command which was
@@ -2833,8 +3196,12 @@ qemudDomainBlockStats (virDomainPtr dom,
const char *qemu_dev_name = NULL;
size_t len;
int i, ret = -1;
- const virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ virDomainObjPtr vm;
virDomainDiskDefPtr disk = NULL;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -2844,7 +3211,7 @@ qemudDomainBlockStats (virDomainPtr dom,
if (!virDomainIsActive (vm)) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("domain is not running"));
- return -1;
+ goto cleanup;
}
for (i = 0 ; i < vm->def->ndisks ; i++) {
@@ -2857,7 +3224,7 @@ qemudDomainBlockStats (virDomainPtr dom,
if (!disk) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("invalid path: %s"), path);
- return -1;
+ goto cleanup;
}
qemu_dev_name = qemudDiskDeviceName(dom, disk);
@@ -2868,7 +3235,7 @@ qemudDomainBlockStats (virDomainPtr dom,
if (qemudMonitorCommand (vm, "info blockstats", &info) < 0) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("'info blockstats' command
failed"));
- goto out;
+ goto cleanup;
}
DEBUG ("info blockstats reply: %s", info);
@@ -2881,7 +3248,7 @@ qemudDomainBlockStats (virDomainPtr dom,
qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s",
_("'info blockstats' not supported by this
qemu"));
- goto out;
+ goto cleanup;
}
stats->rd_req = -1;
@@ -2933,7 +3300,7 @@ qemudDomainBlockStats (virDomainPtr dom,
p++;
}
ret = 0;
- goto out;
+ goto done;
}
/* Skip to next line. */
@@ -2945,10 +3312,20 @@ qemudDomainBlockStats (virDomainPtr dom,
/* If we reach here then the device was not found. */
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("device not found: %s (%s)"), path, qemu_dev_name);
- out:
+ goto cleanup;
+
+ done:
VIR_FREE(qemu_dev_name);
VIR_FREE(info);
- return ret;
+ virDomainUnlock(vm);
+ return 0;
+
+cleanup:
+ VIR_FREE(qemu_dev_name);
+ VIR_FREE(info);
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int
@@ -2958,25 +3335,29 @@ qemudDomainInterfaceStats (virDomainPtr
{
#ifdef __linux__
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
- int i;
+ virDomainObjPtr vm;
+ int i, ret;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("domain is not running"));
- return -1;
+ goto cleanup;
}
if (!path || path[0] == '\0') {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s", _("NULL or empty path"));
- return -1;
+ goto cleanup;
}
/* Check the path is one of the domain's network interfaces. */
@@ -2988,10 +3369,17 @@ qemudDomainInterfaceStats (virDomainPtr
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
_("invalid path, '%s' is not a known interface"),
path);
+ goto cleanup;
+
+ ok:
+ ret = linuxDomainInterfaceStats (dom->conn, path, stats);
+ virDomainUnlock(vm);
+ return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
return -1;
- ok:
-
- return linuxDomainInterfaceStats (dom->conn, path, stats);
#else
qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
"%s", __FUNCTION__);
@@ -3007,19 +3395,23 @@ qemudDomainBlockPeek (virDomainPtr dom,
unsigned int flags ATTRIBUTE_UNUSED)
{
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
int fd, ret = -1, i;
+
+ qemudDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemudDriverUnlock(driver);
if (!vm) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return -1;
+ goto cleanup;
}
if (!path || path[0] == '\0') {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s", _("NULL or empty path"));
- return -1;
+ goto cleanup;
}
/* Check the path belongs to this domain. */
@@ -3030,7 +3422,7 @@ qemudDomainBlockPeek (virDomainPtr dom,
}
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s", _("invalid path"));
- return -1;
+ goto cleanup;
found:
/* The path is correct, now try to open it and get its size. */
@@ -3055,7 +3447,14 @@ found:
ret = 0;
done:
if (fd >= 0) close (fd);
+
+ virDomainUnlock(vm);
return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
static int
@@ -3065,34 +3464,38 @@ qemudDomainMemoryPeek (virDomainPtr dom,
unsigned int flags)
{
struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ virDomainObjPtr vm;
char cmd[256], *info;
char tmp[] = TEMPDIR "/qemu.mem.XXXXXX";
int fd = -1, ret = -1;
+ qemudDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
+ qemudDriverUnlock(driver);
+
if (flags != VIR_MEMORY_VIRTUAL) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
"%s", _("QEMU driver only supports virtual
memory addrs"));
- return -1;
+ goto cleanup;
}
if (!vm) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
_("no domain with matching id %d"), dom->id);
- return -1;
+ goto cleanup;
}
if (!virDomainIsActive(vm)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("domain is not running"));
- return -1;
+ goto cleanup;
}
/* Create a temporary filename. */
if ((fd = mkstemp (tmp)) == -1) {
qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR,
"%s", strerror (errno));
- return -1;
+ goto cleanup;
}
/* Issue the memsave command. */
@@ -3104,7 +3507,7 @@ qemudDomainMemoryPeek (virDomainPtr dom,
}
DEBUG ("memsave reply: %s", info);
- free (info);
+ VIR_FREE(info);
/* Read the memory file into buffer. */
if (saferead (fd, buffer, size) == (ssize_t) -1) {
@@ -3117,7 +3520,13 @@ done:
done:
if (fd >= 0) close (fd);
unlink (tmp);
+ virDomainUnlock(vm);
return ret;
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:04 a.m.
New subject: [libvirt] PATCH: 4/5: Locking in the LXC driver
This implements the same logic as described for the QEMU driver,
in the LXC driver & its API calls. The concurrency on this is
already very good, since the LXC driver has no long running
code by virtue of not having a 'monitor' console to interact
with.
Daniel
diff --git a/src/lxc_conf.h b/src/lxc_conf.h
--- a/src/lxc_conf.h
+++ b/src/lxc_conf.h
@@ -36,6 +36,8 @@
typedef struct __lxc_driver lxc_driver_t;
struct __lxc_driver {
+ pthread_mutex_t lock;
+
virCapsPtr caps;
virDomainObjList domains;
diff --git a/src/lxc_driver.c b/src/lxc_driver.c
--- a/src/lxc_driver.c
+++ b/src/lxc_driver.c
@@ -52,6 +52,19 @@ static lxc_driver_t *lxc_driver = NULL;
static lxc_driver_t *lxc_driver = NULL;
/* Functions */
+
+static int
+lxcDriverLock(lxc_driver_t *driver)
+{
+ return pthread_mutex_lock(&driver->lock);
+}
+
+static int
+lxcDriverUnlock(lxc_driver_t *driver)
+{
+ return pthread_mutex_unlock(&driver->lock);
+}
+
static const char *lxcProbe(void)
{
@@ -104,8 +117,12 @@ static virDomainPtr lxcDomainLookupByID(
int id)
{
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, id);
+ virDomainObjPtr vm;
virDomainPtr dom;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, id);
+ lxcDriverUnlock(driver);
if (!vm) {
lxcError(conn, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -117,6 +134,7 @@ static virDomainPtr lxcDomainLookupByID(
dom->id = vm->def->id;
}
+ virDomainUnlock(vm);
return dom;
}
@@ -124,8 +142,12 @@ static virDomainPtr lxcDomainLookupByUUI
const unsigned char *uuid)
{
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, uuid);
+ virDomainObjPtr vm;
virDomainPtr dom;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, uuid);
+ lxcDriverUnlock(driver);
if (!vm) {
lxcError(conn, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -137,6 +159,7 @@ static virDomainPtr lxcDomainLookupByUUI
dom->id = vm->def->id;
}
+ virDomainUnlock(vm);
return dom;
}
@@ -144,8 +167,12 @@ static virDomainPtr lxcDomainLookupByNam
const char *name)
{
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
- virDomainObjPtr vm = virDomainFindByName(&driver->domains, name);
+ virDomainObjPtr vm;
virDomainPtr dom;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByName(&driver->domains, name);
+ lxcDriverUnlock(driver);
if (!vm) {
lxcError(conn, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -157,6 +184,7 @@ static virDomainPtr lxcDomainLookupByNam
dom->id = vm->def->id;
}
+ virDomainUnlock(vm);
return dom;
}
@@ -164,9 +192,16 @@ static int lxcListDomains(virConnectPtr
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
int got = 0, i;
- for (i = 0 ; i < driver->domains.count && got < nids ; i++)
+ lxcDriverLock(driver);
+
+ for (i = 0 ; i < driver->domains.count && got < nids ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (virDomainIsActive(driver->domains.objs[i]))
ids[got++] = driver->domains.objs[i]->def->id;
+ virDomainUnlock(driver->domains.objs[i]);
+ }
+
+ lxcDriverUnlock(driver);
return got;
}
@@ -174,9 +209,16 @@ static int lxcNumDomains(virConnectPtr c
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
int n = 0, i;
- for (i = 0 ; i < driver->domains.count ; i++)
+ lxcDriverLock(driver);
+
+ for (i = 0 ; i < driver->domains.count ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (virDomainIsActive(driver->domains.objs[i]))
n++;
+ virDomainUnlock(driver->domains.objs[i]);
+ }
+
+ lxcDriverUnlock(driver);
return n;
}
@@ -186,21 +228,28 @@ static int lxcListDefinedDomains(virConn
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
int got = 0, i;
+ lxcDriverLock(driver);
+
for (i = 0 ; i < driver->domains.count && got < nnames ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (!virDomainIsActive(driver->domains.objs[i])) {
if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) {
lxcError(conn, NULL, VIR_ERR_NO_MEMORY,
"%s", _("failed to allocate space for VM name
string"));
+ virDomainUnlock(driver->domains.objs[i]);
goto cleanup;
}
}
+ virDomainUnlock(driver->domains.objs[i]);
}
+ lxcDriverUnlock(driver);
return got;
cleanup:
for (i = 0 ; i < got ; i++)
VIR_FREE(names[i]);
+ lxcDriverUnlock(driver);
return -1;
}
@@ -209,9 +258,14 @@ static int lxcNumDefinedDomains(virConne
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
int n = 0, i;
- for (i = 0 ; i < driver->domains.count ; i++)
+ lxcDriverLock(driver);
+ for (i = 0 ; i < driver->domains.count ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (!virDomainIsActive(driver->domains.objs[i]))
n++;
+ virDomainUnlock(driver->domains.objs[i]);
+ }
+ lxcDriverUnlock(driver);
return n;
}
@@ -225,27 +279,28 @@ static virDomainPtr lxcDomainDefine(virC
virDomainObjPtr vm;
virDomainPtr dom;
+ lxcDriverLock(driver);
+
if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
- return NULL;
+ goto error;
if ((def->nets != NULL) && !(driver->have_netns)) {
lxcError(conn, NULL, VIR_ERR_NO_SUPPORT,
"%s", _("System lacks NETNS support"));
- virDomainDefFree(def);
- return NULL;
+ goto error;
}
- if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) {
- virDomainDefFree(def);
- return NULL;
- }
+ if (!(vm = virDomainAssignDef(conn, &driver->domains, def)))
+ goto error;
+
vm->persistent = 1;
+ def = NULL;
if (virDomainSaveConfig(conn,
driver->configDir,
vm->newDef ? vm->newDef : vm->def) < 0) {
virDomainRemoveInactive(&driver->domains, vm);
- return NULL;
+ goto error;
}
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
@@ -253,48 +308,73 @@ static virDomainPtr lxcDomainDefine(virC
dom->id = vm->def->id;
}
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
+
return dom;
+
+error:
+ if (def)
+ virDomainDefFree(def);
+
+ lxcDriverUnlock(driver);
+ return NULL;
}
static int lxcDomainUndefine(virDomainPtr dom)
{
lxc_driver_t *driver = (lxc_driver_t *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
- return -1;
+ goto error;
}
if (virDomainIsActive(vm)) {
lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot delete active domain"));
- return -1;
+ goto error;
}
if (!vm->persistent) {
lxcError(dom->conn, dom, VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot undefine transient domain"));
- return -1;
+ goto error;
}
if (virDomainDeleteConfig(dom->conn,
driver->configDir,
driver->autostartDir,
vm) <0)
- return -1;
+ goto error;
+ virDomainUnlock(vm);
virDomainRemoveInactive(&driver->domains, vm);
+ lxcDriverUnlock(driver);
return 0;
+
+error:
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
+ return -1;
}
static int lxcDomainGetInfo(virDomainPtr dom,
virDomainInfoPtr info)
{
lxc_driver_t *driver = (lxc_driver_t *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ lxcDriverUnlock(driver);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
@@ -314,13 +394,19 @@ static int lxcDomainGetInfo(virDomainPtr
info->memory = vm->def->memory;
info->nrVirtCpu = 1;
+ virDomainUnlock(vm);
return 0;
}
static char *lxcGetOSType(virDomainPtr dom)
{
lxc_driver_t *driver = (lxc_driver_t *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+ char *ret;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ lxcDriverUnlock(driver);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
@@ -328,14 +414,21 @@ static char *lxcGetOSType(virDomainPtr d
return NULL;
}
- return strdup(vm->def->os.type);
+ ret = strdup(vm->def->os.type);
+ virDomainUnlock(vm);
+ return ret;
}
static char *lxcDomainDumpXML(virDomainPtr dom,
int flags)
{
lxc_driver_t *driver = (lxc_driver_t *)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ virDomainObjPtr vm;
+ char *ret;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ lxcDriverUnlock(driver);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
@@ -343,10 +436,12 @@ static char *lxcDomainDumpXML(virDomainP
return NULL;
}
- return virDomainDefFormat(dom->conn,
- (flags & VIR_DOMAIN_XML_INACTIVE) &&
- vm->newDef ? vm->newDef : vm->def,
+ ret = virDomainDefFormat(dom->conn,
+ (flags & VIR_DOMAIN_XML_INACTIVE) &&
+ vm->newDef ? vm->newDef : vm->def,
flags);
+ virDomainUnlock(vm);
+ return ret;
}
@@ -590,19 +685,29 @@ static void lxcMonitorEvent(int fd,
virDomainObjPtr vm = NULL;
unsigned int i;
+ lxcDriverLock(driver);
+
for (i = 0 ; i < driver->domains.count ; i++) {
+ virDomainLock(driver->domains.objs[i]);
if (driver->domains.objs[i]->monitor == fd) {
vm = driver->domains.objs[i];
break;
}
+ virDomainUnlock(driver->domains.objs[i]);
}
+
if (!vm) {
virEventRemoveHandle(fd);
- return;
+ goto cleanup;
}
if (lxcVmTerminate(NULL, driver, vm, SIGINT) < 0)
virEventRemoveHandle(fd);
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
}
@@ -851,7 +956,10 @@ static int lxcDomainStart(virDomainPtr d
int rc = -1;
virConnectPtr conn = dom->conn;
lxc_driver_t *driver = (lxc_driver_t *)(conn->privateData);
- virDomainObjPtr vm = virDomainFindByName(&driver->domains, dom->name);
+ virDomainObjPtr vm;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByName(&driver->domains, dom->name);
if (!vm) {
lxcError(conn, dom, VIR_ERR_INVALID_DOMAIN,
@@ -868,6 +976,9 @@ static int lxcDomainStart(virDomainPtr d
rc = lxcVmStart(conn, driver, vm);
cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
return rc;
}
@@ -890,6 +1001,8 @@ lxcDomainCreateAndStart(virConnectPtr co
virDomainDefPtr def;
virDomainPtr dom = NULL;
+ lxcDriverLock(driver);
+
if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
goto return_point;
@@ -917,6 +1030,9 @@ lxcDomainCreateAndStart(virConnectPtr co
}
return_point:
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
return dom;
}
@@ -931,15 +1047,25 @@ static int lxcDomainShutdown(virDomainPt
static int lxcDomainShutdown(virDomainPtr dom)
{
lxc_driver_t *driver = (lxc_driver_t*)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ virDomainObjPtr vm;
+ int ret = -1;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
_("no domain with id %d"), dom->id);
- return -1;
+ goto cleanup;
}
- return lxcVmTerminate(dom->conn, driver, vm, 0);
+ ret = lxcVmTerminate(dom->conn, driver, vm, 0);
+
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
+ return ret;
}
@@ -954,15 +1080,24 @@ static int lxcDomainDestroy(virDomainPtr
static int lxcDomainDestroy(virDomainPtr dom)
{
lxc_driver_t *driver = (lxc_driver_t*)dom->conn->privateData;
- virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+ virDomainObjPtr vm;
+ int ret = -1;
+
+ lxcDriverLock(driver);
+ vm = virDomainFindByID(&driver->domains, dom->id);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
_("no domain with id %d"), dom->id);
- return -1;
+ goto cleanup;
}
- return lxcVmTerminate(dom->conn, driver, vm, SIGKILL);
+ ret = lxcVmTerminate(dom->conn, driver, vm, SIGKILL);
+cleanup:
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
+ return ret;
}
static int lxcCheckNetNsSupport(void)
@@ -990,35 +1125,31 @@ static int lxcStartup(void)
return -1;
}
- if (VIR_ALLOC(lxc_driver) < 0) {
- return -1;
- }
-
/* Check that this is a container enabled kernel */
if(lxcContainerAvailable(0) < 0)
return -1;
+ if (VIR_ALLOC(lxc_driver) < 0)
+ return -1;
+
+ pthread_mutex_init(&lxc_driver->lock, NULL);
+ lxcDriverLock(lxc_driver);
+
lxc_driver->have_netns = lxcCheckNetNsSupport();
/* Call function to load lxc driver configuration information */
- if (lxcLoadDriverConfig(lxc_driver) < 0) {
- lxcShutdown();
- return -1;
- }
+ if (lxcLoadDriverConfig(lxc_driver) < 0)
+ goto error;
- if ((lxc_driver->caps = lxcCapsInit()) == NULL) {
- lxcShutdown();
- return -1;
- }
+ if ((lxc_driver->caps = lxcCapsInit()) == NULL)
+ goto error;
if (virDomainLoadAllConfigs(NULL,
lxc_driver->caps,
&lxc_driver->domains,
lxc_driver->configDir,
- lxc_driver->autostartDir) < 0) {
- lxcShutdown();
- return -1;
- }
+ lxc_driver->autostartDir) < 0)
+ goto error;
for (i = 0 ; i < lxc_driver->domains.count ; i++) {
virDomainObjPtr vm = lxc_driver->domains.objs[i];
@@ -1058,7 +1189,13 @@ static int lxcStartup(void)
}
}
+ lxcDriverUnlock(lxc_driver);
return 0;
+
+error:
+ lxcDriverUnlock(lxc_driver);
+ lxcShutdown();
+ return -1;
}
static void lxcFreeDriver(lxc_driver_t *driver)
@@ -1075,7 +1212,9 @@ static int lxcShutdown(void)
if (lxc_driver == NULL)
return(-1);
+ lxcDriverLock(lxc_driver);
virDomainObjListFree(&lxc_driver->domains);
+ lxcDriverUnlock(lxc_driver);
lxcFreeDriver(lxc_driver);
lxc_driver = NULL;
@@ -1091,17 +1230,20 @@ static int lxcShutdown(void)
*/
static int
lxcActive(void) {
- unsigned int i;
+ unsigned int i, active = 0;
if (lxc_driver == NULL)
return(0);
- for (i = 0 ; i < lxc_driver->domains.count ; i++)
+ lxcDriverLock(lxc_driver);
+ for (i = 0 ; i < lxc_driver->domains.count ; i++) {
+ virDomainLock(lxc_driver->domains.objs[i]);
if (virDomainIsActive(lxc_driver->domains.objs[i]))
- return 1;
+ active = 1;
+ virDomainUnlock(lxc_driver->domains.objs[i]);
+ }
- /* Otherwise we're happy to deal with a shutdown */
- return 0;
+ return active;
}
static int lxcVersion(virConnectPtr conn, unsigned long *version)
@@ -1137,84 +1279,99 @@ static char *lxcGetSchedulerType(virDoma
return strdup("posix");
}
-static int lxcSetSchedulerParameters(virDomainPtr _domain,
+static int lxcSetSchedulerParameters(virDomainPtr domain,
virSchedParameterPtr params,
int nparams)
{
+ lxc_driver_t *driver = domain->conn->privateData;
int i;
- int rc;
virCgroupPtr group;
- virDomainObjPtr domain;
+ virDomainObjPtr vm;
if (virCgroupHaveSupport() != 0)
return 0;
- domain = virDomainFindByUUID(&lxc_driver->domains, _domain->uuid);
- if (domain == NULL) {
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&lxc_driver->domains, domain->uuid);
+ lxcDriverUnlock(driver);
+
+ if (vm == NULL) {
lxcError(NULL, _domain, VIR_ERR_INTERNAL_ERROR,
- _("No such domain %s"), _domain->uuid);
- return -EINVAL;
+ _("No such domain %s"), domain->uuid);
+ goto error;
}
- rc = virCgroupForDomain(domain->def, "lxc", &group);
- if (rc != 0)
- return rc;
+ if (virCgroupForDomain(vm->def, "lxc", &group) != 0)
+ goto error;
for (i = 0; i < nparams; i++) {
virSchedParameterPtr param = ¶ms[i];
if (STREQ(param->field, "cpu_shares")) {
- rc = virCgroupSetCpuShares(group, params[i].value.ui);
+ if (virCgroupSetCpuShares(group, params[i].value.ui) != 0)
+ goto error;
} else {
- lxcError(NULL, _domain, VIR_ERR_INVALID_ARG,
+ lxcError(NULL, domain, VIR_ERR_INVALID_ARG,
_("Invalid parameter `%s'"), param->field);
- rc = -ENOENT;
- goto out;
+ goto error;
}
}
- rc = 0;
-out:
virCgroupFree(&group);
+ return 0;
- return rc;
+error:
+ if (group)
+ virCgroupFree(&group);
+ if (vm)
+ virDomainUnlock(vm);
+ lxcDriverUnlock(driver);
+ return -1;
}
-static int lxcGetSchedulerParameters(virDomainPtr _domain,
+static int lxcGetSchedulerParameters(virDomainPtr domain,
virSchedParameterPtr params,
int *nparams)
{
- int rc = 0;
+ lxc_driver_t *driver = domain->conn->privateData;
virCgroupPtr group;
- virDomainObjPtr domain;
+ virDomainObjPtr vm;
if (virCgroupHaveSupport() != 0)
return 0;
if ((*nparams) != 1) {
- lxcError(NULL, _domain, VIR_ERR_INVALID_ARG,
+ lxcError(NULL, domain, VIR_ERR_INVALID_ARG,
"%s", _("Invalid parameter count"));
- return -1;
+ goto error;
}
- domain = virDomainFindByUUID(&lxc_driver->domains, _domain->uuid);
- if (domain == NULL) {
- lxcError(NULL, _domain, VIR_ERR_INTERNAL_ERROR,
- _("No such domain %s"), _domain->uuid);
- return -ENOENT;
+ lxcDriverLock(driver);
+ vm = virDomainFindByUUID(&lxc_driver->domains, domain->uuid);
+ lxcDriverUnlock(driver);
+ if (vm == NULL) {
+ lxcError(NULL, domain, VIR_ERR_INTERNAL_ERROR,
+ _("No such domain %s"), domain->uuid);
+ goto error;
}
- rc = virCgroupForDomain(domain->def, "lxc", &group);
- if (rc != 0)
- return rc;
+ if (virCgroupForDomain(vm->def, "lxc", &group) != 0)
+ goto error;
- rc = virCgroupGetCpuShares(group, (unsigned long *)¶ms[0].value.ul);
+ if (virCgroupGetCpuShares(group, (unsigned long *)¶ms[0].value.ul) != 0)
+ goto error;
+
strncpy(params[0].field, "cpu_shares", sizeof(params[0].field));
params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG;
virCgroupFree(&group);
- return rc;
+ return 0;
+
+error:
+ if (vm)
+ virDomainUnlock(vm);
+ return -1;
}
/* Function Tables */
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:53 a.m.
New subject: [libvirt] PATCH: 4/5: Locking in the LXC driver
DB> @@ -104,8 +117,12 @@ static virDomainPtr lxcDomainLookupByID(
DB> int id)
DB> {
DB> lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
DB> - virDomainObjPtr vm = virDomainFindByID(&driver->domains, id);
DB> + virDomainObjPtr vm;
DB> virDomainPtr dom;
DB> +
DB> + lxcDriverLock(driver);
DB> + vm = virDomainFindByID(&driver->domains, id);
DB> + lxcDriverUnlock(driver);
DB> if (!vm) {
DB> lxcError(conn, NULL, VIR_ERR_NO_DOMAIN, NULL);
DB> @@ -117,6 +134,7 @@ static virDomainPtr lxcDomainLookupByID(
dom-> id = vm->def->id;
DB> }
DB> + virDomainUnlock(vm);
DB> return dom;
DB> }
Can you explain why you're unlocking the vm in all of these functions
without first the corresponding lock operation?
DB> @@ -1091,17 +1230,20 @@ static int lxcShutdown(void)
DB> */
DB> static int
DB> lxcActive(void) {
DB> - unsigned int i;
DB> + unsigned int i, active = 0;
DB> if (lxc_driver == NULL)
DB> return(0);
DB> - for (i = 0 ; i < lxc_driver->domains.count ; i++)
DB> + lxcDriverLock(lxc_driver);
DB> + for (i = 0 ; i < lxc_driver->domains.count ; i++) {
DB> + virDomainLock(lxc_driver->domains.objs[i]);
DB> if (virDomainIsActive(lxc_driver->domains.objs[i]))
DB> - return 1;
DB> + active = 1;
DB> + virDomainUnlock(lxc_driver->domains.objs[i]);
DB> + }
DB> - /* Otherwise we're happy to deal with a shutdown */
DB> - return 0;
DB> + return active;
DB> }
It looks to me like you're taking the driver lock and not releasing it
on exit.
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
9:56 a.m.
New subject: [libvirt] PATCH: 4/5: Locking in the LXC driver
On Fri, Oct 17, 2008 at 07:53:06AM -0700, Dan Smith wrote:
DB> @@ -104,8 +117,12 @@ static virDomainPtr lxcDomainLookupByID(
DB> int id)
DB> {
DB> lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
DB> - virDomainObjPtr vm = virDomainFindByID(&driver->domains, id);
DB> + virDomainObjPtr vm;
DB> virDomainPtr dom;
DB> +
DB> + lxcDriverLock(driver);
DB> + vm = virDomainFindByID(&driver->domains, id);
DB> + lxcDriverUnlock(driver);
DB> if (!vm) {
DB> lxcError(conn, NULL, VIR_ERR_NO_DOMAIN, NULL);
DB> @@ -117,6 +134,7 @@ static virDomainPtr lxcDomainLookupByID(
dom-> id = vm->def->id;
DB> }
DB> + virDomainUnlock(vm);
DB> return dom;
DB> }
Can you explain why you're unlocking the vm in all of these functions
without first the corresponding lock operation?
You didn't read the previous mail in this sequence :-P
http://www.redhat.com/archives/libvir-list/2008-October/msg00419.html
[quote]
* Domain lookup methods
virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms,
int id);
virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
const unsigned char *uuid);
virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
const char *name);
The driver must hold its global lock to protect the virDomainObjListPtr
object. The returned virDomainObjPtr instance will be locked at which
point the driver can optionall release its global lock
[/quote]
DB> @@ -1091,17 +1230,20 @@ static int lxcShutdown(void)
DB> */
DB> static int
DB> lxcActive(void) {
DB> - unsigned int i;
DB> + unsigned int i, active = 0;
DB> if (lxc_driver == NULL)
DB> return(0);
DB> - for (i = 0 ; i < lxc_driver->domains.count ; i++)
DB> + lxcDriverLock(lxc_driver);
DB> + for (i = 0 ; i < lxc_driver->domains.count ; i++) {
DB> + virDomainLock(lxc_driver->domains.objs[i]);
DB> if (virDomainIsActive(lxc_driver->domains.objs[i]))
DB> - return 1;
DB> + active = 1;
DB> + virDomainUnlock(lxc_driver->domains.objs[i]);
DB> + }
DB> - /* Otherwise we're happy to deal with a shutdown */
DB> - return 0;
DB> + return active;
DB> }
It looks to me like you're taking the driver lock and not releasing it
on exit.
Yes, well spotted.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:15 a.m.
New subject: [libvirt] PATCH: 4/5: Locking in the LXC driver
DB> You didn't read the previous mail in this sequence :-P
Indeed. The temperamental mail servers have only graced me with 1, 4,
and 5 so far. I should have checked the archives.
Aside from the other issue, I'm fine with the patch :)
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
9:06 a.m.
New subject: [libvirt] PATCH: 5/5: Locking for the virNetworkObjPtr instances
This implements locking in the virNetworkObjPtr objects. This follows
exactly the same model as the virDomainObjPtr locking.
There still needs to be code in the network driver itself to make use
of this locking, so this patch is a work in progress
Daniel
diff --git a/src/network_conf.c b/src/network_conf.c
--- a/src/network_conf.c
+++ b/src/network_conf.c
@@ -52,26 +52,46 @@ VIR_ENUM_IMPL(virNetworkForward,
__virReportErrorHelper(conn, VIR_FROM_NETWORK, code, __FILE__, \
__FUNCTION__, __LINE__, fmt)
+/**
+ * @virNetworkFindByUUID:
+ *
+ * Caller must hold exclusive lock over 'nets'
+ *
+ * Returns a virNetworkObjPtr locked for exclusive access, or NULL
+ */
virNetworkObjPtr virNetworkFindByUUID(const virNetworkObjListPtr nets,
const unsigned char *uuid)
{
unsigned int i;
- for (i = 0 ; i < nets->count ; i++)
+ for (i = 0 ; i < nets->count ; i++) {
+ virNetworkLock(nets->objs[i]);
if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN))
return nets->objs[i];
+ virNetworkUnlock(nets->objs[i]);
+ }
return NULL;
}
+/**
+ * @virNetworkFindByName:
+ *
+ * Caller must hold exclusive lock over 'nets'
+ *
+ * Returns a virNetworkObjPtr locked for exclusive access, or NULL
+ */
virNetworkObjPtr virNetworkFindByName(const virNetworkObjListPtr nets,
const char *name)
{
unsigned int i;
- for (i = 0 ; i < nets->count ; i++)
+ for (i = 0 ; i < nets->count ; i++) {
+ virNetworkLock(nets->objs[i]);
if (STREQ(nets->objs[i]->def->name, name))
return nets->objs[i];
+ virNetworkUnlock(nets->objs[i]);
+ }
return NULL;
}
@@ -108,6 +128,13 @@ void virNetworkDefFree(virNetworkDefPtr
VIR_FREE(def);
}
+
+/**
+ * @virDomainObjFree
+ *
+ * Caller should not hold lock on 'net', but must
+ * also ensure no other thread can have an active lock
+ */
void virNetworkObjFree(virNetworkObjPtr net)
{
if (!net)
@@ -122,16 +149,35 @@ void virNetworkObjFree(virNetworkObjPtr
VIR_FREE(net);
}
+/**
+ * @virNetworkObjListFree
+ *
+ * Caller must hold exclusive lock over 'nets'
+ */
void virNetworkObjListFree(virNetworkObjListPtr nets)
{
unsigned int i;
- for (i = 0 ; i < nets->count ; i++)
+ for (i = 0 ; i < nets->count ; i++) {
+ /* Lock+unlock ensures no one is still using this dom */
+ virNetworkLock(nets->objs[i]);
+ virNetworkUnlock(nets->objs[i]);
+
virNetworkObjFree(nets->objs[i]);
+ }
VIR_FREE(nets->objs);
nets->count = 0;
}
+
+
+/**
+ * @virNetworkAssignDef
+ *
+ * Caller must hold exclusive lock over 'nets'.
+ *
+ * The return network object will be locked
+ */
virNetworkObjPtr virNetworkAssignDef(virConnectPtr conn,
virNetworkObjListPtr nets,
@@ -158,6 +204,7 @@ virNetworkObjPtr virNetworkAssignDef(vir
}
network->def = def;
+ pthread_mutex_init(&network->lock, NULL);
if (VIR_REALLOC_N(nets->objs, nets->count + 1) < 0) {
virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
@@ -168,14 +215,27 @@ virNetworkObjPtr virNetworkAssignDef(vir
nets->objs[nets->count] = network;
nets->count++;
+ virNetworkLock(network);
+
return network;
}
+
+/**
+ * @virNetworkRemoveInactive:
+ *
+ * Caller must hold exclusive lock over 'nets', but
+ * 'net' must be unlocked
+ */
void virNetworkRemoveInactive(virNetworkObjListPtr nets,
const virNetworkObjPtr net)
{
unsigned int i;
+
+ /* Ensure no other thread holds a lock on dom */
+ virNetworkLock(net);
+ virNetworkUnlock(net);
for (i = 0 ; i < nets->count ; i++) {
if (nets->objs[i] == net) {
@@ -769,6 +829,8 @@ int virNetworkLoadAllConfigs(virConnectP
}
while ((entry = readdir(dir))) {
+ virNetworkObjPtr net;
+
if (entry->d_name[0] == '.')
continue;
@@ -777,11 +839,15 @@ int virNetworkLoadAllConfigs(virConnectP
/* NB: ignoring errors, so one malformed config doesn't
kill the whole process */
- virNetworkLoadConfig(conn,
- nets,
- configDir,
- autostartDir,
- entry->d_name);
+ net = virNetworkLoadConfig(conn,
+ nets,
+ configDir,
+ autostartDir,
+ entry->d_name);
+ if (net) {
+ net->persistent = 1;
+ virNetworkUnlock(net);
+ }
}
closedir(dir);
diff --git a/src/network_conf.h b/src/network_conf.h
--- a/src/network_conf.h
+++ b/src/network_conf.h
@@ -78,6 +78,8 @@ typedef struct _virNetworkObj virNetwork
typedef struct _virNetworkObj virNetworkObj;
typedef virNetworkObj *virNetworkObjPtr;
struct _virNetworkObj {
+ pthread_mutex_t lock;
+
pid_t dnsmasqPid;
unsigned int active : 1;
unsigned int autostart : 1;
@@ -103,6 +105,19 @@ virNetworkIsActive(const virNetworkObjPt
return net->active;
}
+static inline int
+virNetworkLock(virNetworkObjPtr net)
+{
+ return pthread_mutex_lock(&net->lock);
+}
+
+
+static inline int
+virNetworkUnlock(virNetworkObjPtr net)
+{
+ return pthread_mutex_unlock(&net->lock);
+}
+
virNetworkObjPtr virNetworkFindByUUID(const virNetworkObjListPtr nets,
const unsigned char *uuid);
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5880
days inactive
5880
days old
8 comments
2 participants
participants (2)
-
Dan Smith -
Daniel P. Berrange