External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-10-05 05:45:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix misc thread locking bugs / bogus warnings

Browse Source 
Fix all thread locking bugs reported by object-locking test
case.

NB, some of the driver locking is getting too coarse. Driver
mutexes really need to be turned into RW locks instead to
significantly increase concurrency.

* src/lxc_driver.c: Fix useof driver when unlocked in the methods
  lxcDomainGetInfo, lxcSetSchedulerParameters, and
  lxcGetSchedulerParameters
* src/opennebula/one_driver.c: Fix missing unlock in oneDomainUndefine.
  Fix use of driver when unlocked in oneDomainGetInfo,
  oneGetOSType, oneDomainShutdown
* src/qemu_driver.c: Fix use of driver when unlocked in
  qemudDomainSavem, qemuGetSchedulerType, qemuSetSchedulerParameters
  and qemuGetSchedulerParameters
* src/storage_driver.c: Re-work storagePoolCreate to avoid bogus
  lock checking warning. Re-work storageVolumeCreateXMLFrom to
  remove a potential NULL de-reference & avoid bogus lock check
  warnings
* src/test.c: Remove testDomainAssignDef since it break lock chekc
  warnings.
* tests/object-locking.ml: Add oneDriverLock, oneDriverUnlock
  and one_driver_t methods/types to allow lock checking on the
   OpenNebula drivers
This commit is contained in:
Daniel P. Berrange 2009-09-02 14:02:06 +01:00
parent e52d608ddf
commit 5c8d3d3bca
6 changed files with 93 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/lxc_driver.c
View File

@ -439,7 +439,6 @@ static int lxcDomainGetInfo(virDomainPtr dom,
lxcDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
lxcDriverUnlock(driver);
if (!vm) {
lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
@ -470,6 +469,7 @@ static int lxcDomainGetInfo(virDomainPtr dom,
ret = 0;
cleanup:
lxcDriverUnlock(driver);
if (cgroup)
virCgroupFree(&cgroup);
if (vm)
@ -1667,7 +1667,6 @@ static int lxcSetSchedulerParameters(virDomainPtr domain,
lxcDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, domain->uuid);
lxcDriverUnlock(driver);
if (vm == NULL) {
lxcError(NULL, domain, VIR_ERR_INTERNAL_ERROR,
@ -1698,6 +1697,7 @@ static int lxcSetSchedulerParameters(virDomainPtr domain,
ret = 0;
cleanup:
lxcDriverUnlock(driver);
virCgroupFree(&group);
if (vm)
virDomainObjUnlock(vm);
@ -1725,7 +1725,6 @@ static int lxcGetSchedulerParameters(virDomainPtr domain,
lxcDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, domain->uuid);
lxcDriverUnlock(driver);
if (vm == NULL) {
lxcError(NULL, domain, VIR_ERR_INTERNAL_ERROR,
@ -1745,6 +1744,7 @@ static int lxcGetSchedulerParameters(virDomainPtr domain,
ret = 0;
cleanup:
lxcDriverUnlock(driver);
virCgroupFree(&group);
if (vm)
virDomainObjUnlock(vm);

35
src/opennebula/one_driver.c
View File

@ -310,6 +310,8 @@ static int oneDomainUndefine(virDomainPtr dom)
ret=0;
return_point:
if (vm)
virDomainObjUnlock(vm);
oneDriverUnlock(driver);
return ret;
}
@ -392,9 +394,9 @@ static int oneDomainGetInfo(virDomainPtr dom,
static char *oneGetOSType(virDomainPtr dom)
{
one_driver_t *driver = (one_driver_t *)dom->conn->privateData;
virDomainObjPtr vm = NULL;
char *ret = NULL;
oneDriverLock(driver);
vm =virDomainFindByUUID(&driver->domains, dom->uuid);
@ -402,11 +404,17 @@ static char *oneGetOSType(virDomainPtr dom)
if (!vm) {
oneError(dom->conn,dom, VIR_ERR_INVALID_DOMAIN,
"%s", _("no domain with matching uuid"));
return NULL;
goto cleanup;
}
virDomainObjUnlock(vm);
return strdup(vm->def->os.type);
ret = strdup(vm->def->os.type);
if (!ret)
virReportOOMError(dom->conn);
cleanup:
if (vm)
virDomainObjUnlock(vm);
return ret;
}
static int oneDomainStart(virDomainPtr dom)
@ -499,24 +507,25 @@ static int oneDomainShutdown(virDomainPtr dom)
int ret=-1;
oneDriverLock(driver);
if((vm=virDomainFindByID(&driver->domains, dom->id))) {
if(!(c_oneShutdown(vm->pid) ) ) {
vm->state=VIR_DOMAIN_SHUTDOWN;
ret= 0;
goto return_point;
}
if (!(vm=virDomainFindByID(&driver->domains, dom->id))) {
oneError(dom->conn,dom, VIR_ERR_INVALID_DOMAIN,
_("no domain with id %d"), dom->id);
goto return_point;
}
if (c_oneShutdown(vm->pid)) {
oneError(dom->conn, dom, VIR_ERR_OPERATION_INVALID,
_("Wrong state to perform action"));
goto return_point;
}
oneError(dom->conn,dom, VIR_ERR_INVALID_DOMAIN,
_("no domain with id %d"), dom->id);
goto return_point;
vm->state=VIR_DOMAIN_SHUTDOWN;
ret= 0;
if (!vm->persistent) {
virDomainRemoveInactive(&driver->domains, vm);
vm = NULL;
}
return_point:
if(vm)
virDomainObjUnlock(vm);

30
src/qemu_driver.c
View File

@ -3660,7 +3660,7 @@ static int qemudDomainSave(virDomainPtr dom,
const char *path)
{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
virDomainObjPtr vm = NULL;
char *command = NULL;
char *info = NULL;
int fd = -1;
@ -3675,6 +3675,7 @@ static int qemudDomainSave(virDomainPtr dom,
memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
header.version = QEMUD_SAVE_VERSION;
qemuDriverLock(driver);
if (driver->saveImageFormat == NULL)
header.compressed = QEMUD_SAVE_FORMAT_RAW;
else {
@ -3684,11 +3685,10 @@ static int qemudDomainSave(virDomainPtr dom,
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("Invalid save image format specified "
"in configuration file"));
return -1;
goto cleanup;
}
}
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (!vm) {
@ -4510,7 +4510,9 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn,
goto cleanup;
}
qemuDriverLock(driver);
def = qemuParseCommandLineString(conn, driver->caps, config);
qemuDriverUnlock(driver);
if (!def)
goto cleanup;
@ -6266,12 +6268,13 @@ static char *qemuGetSchedulerType(virDomainPtr dom,
int *nparams)
{
struct qemud_driver *driver = dom->conn->privateData;
char *ret;
char *ret = NULL;
qemuDriverLock(driver);
if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
__FUNCTION__);
return NULL;
goto cleanup;
}
if (nparams)
@ -6280,6 +6283,9 @@ static char *qemuGetSchedulerType(virDomainPtr dom,
ret = strdup("posix");
if (!ret)
virReportOOMError(dom->conn);
cleanup:
qemuDriverUnlock(driver);
return ret;
}
@ -6293,15 +6299,14 @@ static int qemuSetSchedulerParameters(virDomainPtr dom,
virDomainObjPtr vm = NULL;
int ret = -1;
qemuDriverLock(driver);
if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
__FUNCTION__);
return -1;
goto cleanup;
}
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
qemuDriverUnlock(driver);
if (vm == NULL) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
@ -6344,6 +6349,7 @@ cleanup:
virCgroupFree(&group);
if (vm)
virDomainObjUnlock(vm);
qemuDriverUnlock(driver);
return ret;
}
@ -6358,21 +6364,20 @@ static int qemuGetSchedulerParameters(virDomainPtr dom,
int ret = -1;
int rc;
qemuDriverLock(driver);
if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
__FUNCTION__);
return -1;
goto cleanup;
}
if ((*nparams) != 1) {
qemudReportError(dom->conn, domain, NULL, VIR_ERR_INVALID_ARG,
"%s", _("Invalid parameter count"));
return -1;
goto cleanup;
}
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
qemuDriverUnlock(driver);
if (vm == NULL) {
qemudReportError(dom->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR,
@ -6402,6 +6407,7 @@ cleanup:
virCgroupFree(&group);
if (vm)
virDomainObjUnlock(vm);
qemuDriverUnlock(driver);
return ret;
}

35
src/storage_driver.c
View File

@ -489,24 +489,27 @@ storagePoolCreate(virConnectPtr conn,
def = NULL;
if (backend->startPool &&
backend->startPool(conn, pool) < 0)
backend->startPool(conn, pool) < 0) {
virStoragePoolObjRemove(&driver->pools, pool);
pool = NULL;
goto cleanup;
}
if (backend->refreshPool(conn, pool) < 0) {
if (backend->stopPool)
backend->stopPool(conn, pool);
virStoragePoolObjRemove(&driver->pools, pool);
pool = NULL;
goto cleanup;
}
pool->active = 1;
ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
virStoragePoolObjUnlock(pool);
pool = NULL;
cleanup:
virStoragePoolDefFree(def);
if (pool)
virStoragePoolObjRemove(&driver->pools, pool);
virStoragePoolObjUnlock(pool);
storageDriverUnlock(driver);
return ret;
}
@ -1319,27 +1322,23 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
virStorageBackendPtr backend;
virStorageVolDefPtr origvol = NULL, newvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
int buildret, diffpool;
diffpool = !STREQ(obj->name, vobj->pool);
int buildret;
storageDriverLock(driver);
pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
if (diffpool) {
if (pool && STRNEQ(obj->name, vobj->pool)) {
virStoragePoolObjUnlock(pool);
origpool = virStoragePoolObjFindByName(&driver->pools, vobj->pool);
virStoragePoolObjLock(pool);
} else
origpool = pool;
}
storageDriverUnlock(driver);
if (!pool) {
virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
"%s", _("no storage pool with matching uuid"));
goto cleanup;
}
if (diffpool && !origpool) {
if (STRNEQ(obj->name, vobj->pool) && !origpool) {
virStorageReportError(obj->conn, VIR_ERR_NO_STORAGE_POOL,
_("no storage pool with matching name '%s'"),
vobj->pool);
@ -1352,7 +1351,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
goto cleanup;
}
if (diffpool && !virStoragePoolObjIsActive(origpool)) {
if (origpool && !virStoragePoolObjIsActive(origpool)) {
virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
"%s", _("storage pool is not active"));
goto cleanup;
@ -1361,7 +1360,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
goto cleanup;
origvol = virStorageVolDefFindByName(origpool, vobj->name);
origvol = virStorageVolDefFindByName(origpool ? origpool : pool, vobj->name);
if (!origvol) {
virStorageReportError(obj->conn, VIR_ERR_NO_STORAGE_VOL,
_("no storage vol with matching name '%s'"),
@ -1427,7 +1426,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
newvol->building = 1;
virStoragePoolObjUnlock(pool);
if (diffpool) {
if (origpool) {
origpool->asyncjobs++;
virStoragePoolObjUnlock(origpool);
}
@ -1436,7 +1435,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
storageDriverLock(driver);
virStoragePoolObjLock(pool);
if (diffpool)
if (origpool)
virStoragePoolObjLock(origpool);
storageDriverUnlock(driver);
@ -1445,7 +1444,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
newvol = NULL;
pool->asyncjobs--;
if (diffpool) {
if (origpool) {
origpool->asyncjobs--;
virStoragePoolObjUnlock(origpool);
origpool = NULL;
@ -1467,7 +1466,7 @@ cleanup:
virStorageVolDefFree(newvol);
if (pool)
virStoragePoolObjUnlock(pool);
if (diffpool && origpool)
if (origpool)
virStoragePoolObjUnlock(origpool);
return ret;
}

55
src/test.c
View File

@ -261,12 +261,10 @@ testDomainGenerateIfname(virConnectPtr conn,
return NULL;
}
static virDomainObjPtr
testDomainAssignDef(virConnectPtr conn,
virDomainObjList *domlist,
virDomainDefPtr domdef)
static int
testDomainGenerateIfnames(virConnectPtr conn,
virDomainDefPtr domdef)
{
virDomainObjPtr domobj = NULL;
int i = 0;
for (i = 0; i < domdef->nnets; i++) {
@ -276,18 +274,15 @@ testDomainAssignDef(virConnectPtr conn,
ifname = testDomainGenerateIfname(conn, domdef);
if (!ifname)
goto error;
return -1;
domdef->nets[i]->ifname = ifname;
}
if (!(domobj = virDomainAssignDef(conn, domlist, domdef)))
goto error;
error:
return domobj;
return 0;
}
static int testOpenDefault(virConnectPtr conn) {
int u;
struct timeval tv;
@ -342,10 +337,11 @@ static int testOpenDefault(virConnectPtr conn) {
defaultDomainXML,
VIR_DOMAIN_XML_INACTIVE)))
goto error;
if (!(domobj = testDomainAssignDef(conn, &privconn->domains, domdef))) {
virDomainDefFree(domdef);
if (testDomainGenerateIfnames(conn, domdef) < 0)
goto error;
}
if (!(domobj = virDomainAssignDef(conn, &privconn->domains, domdef)))
goto error;
domdef = NULL;
domobj->def->id = privconn->nextDomID++;
domobj->state = VIR_DOMAIN_RUNNING;
domobj->persistent = 1;
@ -399,6 +395,7 @@ error:
testDriverUnlock(privconn);
conn->privateData = NULL;
VIR_FREE(privconn);
virDomainDefFree(domdef);
return VIR_DRV_OPEN_ERROR;
}
@ -668,7 +665,8 @@ static int testOpenFromFile(virConnectPtr conn,
goto error;
}
if (!(dom = testDomainAssignDef(conn, &privconn->domains, def))) {
if (testDomainGenerateIfnames(conn, def) < 0 ||
!(dom = virDomainAssignDef(conn, &privconn->domains, def))) {
virDomainDefFree(def);
goto error;
}
@ -980,11 +978,11 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
VIR_DOMAIN_XML_INACTIVE)) == NULL)
goto cleanup;
if ((dom = testDomainAssignDef(conn, &privconn->domains,
def)) == NULL) {
virDomainDefFree(def);
if (testDomainGenerateIfnames(conn, def) < 0)
goto cleanup;
}
if (!(dom = virDomainAssignDef(conn, &privconn->domains, def)))
goto cleanup;
def = NULL;
dom->state = VIR_DOMAIN_RUNNING;
dom->def->id = privconn->nextDomID++;
@ -992,15 +990,17 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
VIR_DOMAIN_EVENT_STARTED,
VIR_DOMAIN_EVENT_STARTED_BOOTED);
ret = virGetDomain(conn, def->name, def->uuid);
ret = virGetDomain(conn, dom->def->name, dom->def->uuid);
if (ret)
ret->id = def->id;
ret->id = dom->def->id;
cleanup:
if (dom)
virDomainObjUnlock(dom);
if (event)
testDomainEventQueue(privconn, event);
if (def)
virDomainDefFree(def);
testDriverUnlock(privconn);
return ret;
}
@ -1531,13 +1531,14 @@ static int testDomainRestore(virConnectPtr conn,
if (!def)
goto cleanup;
if ((dom = testDomainAssignDef(conn, &privconn->domains,
def)) == NULL)
if (testDomainGenerateIfnames(conn, def) < 0)
goto cleanup;
if (!(dom = virDomainAssignDef(conn, &privconn->domains, def)))
goto cleanup;
def = NULL;
dom->state = VIR_DOMAIN_RUNNING;
dom->def->id = privconn->nextDomID++;
def = NULL;
event = virDomainEventNewFromObj(dom,
VIR_DOMAIN_EVENT_STARTED,
VIR_DOMAIN_EVENT_STARTED_RESTORED);
@ -1823,10 +1824,10 @@ static virDomainPtr testDomainDefineXML(virConnectPtr conn,
VIR_DOMAIN_XML_INACTIVE)) == NULL)
goto cleanup;
if ((dom = testDomainAssignDef(conn, &privconn->domains,
def)) == NULL) {
if (testDomainGenerateIfnames(conn, def) < 0)
goto cleanup;
if (!(dom = virDomainAssignDef(conn, &privconn->domains, def)))
goto cleanup;
}
def = NULL;
dom->persistent = 1;

7
tests/object-locking.ml
View File

@ -128,7 +128,8 @@ let driverLockMethods = [
"umlDriverLock";
"nodedevDriverLock";
"networkDriverLock";
"storageDriverLock"
"storageDriverLock";
"oneDriverLock"
]
(*
@ -142,7 +143,8 @@ let driverUnlockMethods = [
"umlDriverUnlock";
"nodedevDriverUnlock";
"networkDriverUnlock";
"storageDriverUnlock"
"storageDriverUnlock";
"oneDriverUnlock"
]
(*
@ -159,6 +161,7 @@ let lockableDrivers = [
"virStorageDriverStatePtr";
"network_driver";
"virDeviceMonitorState";
"one_driver_t";
]