[libvirt PATCH 00/10] Automatic mutex management - part 2
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management. Tim Wiederhake (10): vz: Use automatic mutex management vmware: Use automatic mutex management secret: Factor out mutex secret: Use automatic mutex management virlockspace: Use automatic mutex management virtpm: Use automatic mutex management vbox: Use automatic mutex management tools: Use automatic mutex management qemusecuritymock: Use automatic mutex management qemumonitortestutils: Use automatic mutex management src/secret/secret_driver.c | 62 ++++++++------------ src/util/virfirewall.c | 13 ++--- src/util/virlockspace.c | 106 +++++++++++------------------------ src/util/virtpm.c | 39 ++++--------- src/vbox/vbox_common.c | 18 +----- src/vmware/vmware_driver.c | 100 ++++++++++----------------------- src/vz/vz_driver.c | 44 +++++++-------- tests/qemumonitortestutils.c | 65 +++++++++------------ tests/qemusecuritymock.c | 88 ++++++++++------------------- tools/virsh.c | 12 ++-- tools/virt-admin.c | 12 ++-- tools/vsh.c | 8 +-- 12 files changed, 201 insertions(+), 366 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/vz/vz_driver.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index be3e5e4b49..86bc53d631 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -169,11 +169,12 @@ vzGetDriverConnection(void) "%s", _("vz state driver is not active")); return NULL; } - virMutexLock(&vz_driver_lock); - if (!vz_driver) - vz_driver = vzDriverObjNew(); - virObjectRef(vz_driver); - virMutexUnlock(&vz_driver_lock); + + VIR_WITH_MUTEX_LOCK_GUARD(&vz_driver_lock) { + if (!vz_driver) + vz_driver = vzDriverObjNew(); + virObjectRef(vz_driver); + } return vz_driver; } @@ -181,13 +182,13 @@ vzGetDriverConnection(void) void vzDestroyDriverConnection(void) { - struct _vzDriver *driver; - struct _vzConn *privconn_list; + struct _vzDriver *driver = NULL; + struct _vzConn *privconn_list = NULL; - virMutexLock(&vz_driver_lock); - driver = g_steal_pointer(&vz_driver); - privconn_list = g_steal_pointer(&vz_conn_list); - virMutexUnlock(&vz_driver_lock); + VIR_WITH_MUTEX_LOCK_GUARD(&vz_driver_lock) { + driver = g_steal_pointer(&vz_driver); + privconn_list = g_steal_pointer(&vz_conn_list); + } while (privconn_list) { struct _vzConn *privconn = privconn_list; @@ -382,10 +383,10 @@ vzConnectOpen(virConnectPtr conn, if (!(privconn->closeCallback = virNewConnectCloseCallbackData())) goto error; - virMutexLock(&vz_driver_lock); - privconn->next = vz_conn_list; - vz_conn_list = privconn; - virMutexUnlock(&vz_driver_lock); + VIR_WITH_MUTEX_LOCK_GUARD(&vz_driver_lock) { + privconn->next = vz_conn_list; + vz_conn_list = privconn; + } return VIR_DRV_OPEN_SUCCESS; @@ -407,16 +408,15 @@ vzConnectClose(virConnectPtr conn) if (!privconn) return 0; - virMutexLock(&vz_driver_lock); - for (curr = vz_conn_list; curr; prev = &curr->next, curr = curr->next) { - if (curr == privconn) { - *prev = curr->next; - break; + VIR_WITH_MUTEX_LOCK_GUARD(&vz_driver_lock) { + for (curr = vz_conn_list; curr; prev = &curr->next, curr = curr->next) { + if (curr == privconn) { + *prev = curr->next; + break; + } } } - virMutexUnlock(&vz_driver_lock); - virObjectUnref(privconn->closeCallback); virObjectUnref(privconn->driver); VIR_FREE(privconn); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/vmware/vmware_driver.c | 100 ++++++++++++------------------------- 1 file changed, 31 insertions(+), 69 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 998ecdb546..da66f98db0 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -47,19 +47,6 @@ static const char * const vmrun_candidates[] = { #endif /* __APPLE__ */ }; -static void -vmwareDriverLock(struct vmware_driver *driver) -{ - virMutexLock(&driver->lock); -} - -static void -vmwareDriverUnlock(struct vmware_driver *driver) -{ - virMutexUnlock(&driver->lock); -} - - static virDomainObj * vmwareDomObjFromDomainLocked(struct vmware_driver *driver, const unsigned char *uuid) @@ -83,12 +70,9 @@ static virDomainObj * vmwareDomObjFromDomain(struct vmware_driver *driver, const unsigned char *uuid) { - virDomainObj *vm; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - vmwareDriverLock(driver); - vm = vmwareDomObjFromDomainLocked(driver, uuid); - vmwareDriverUnlock(driver); - return vm; + return vmwareDomObjFromDomainLocked(driver, uuid); } @@ -274,10 +258,9 @@ static int vmwareConnectGetVersion(virConnectPtr conn, unsigned long *version) { struct vmware_driver *driver = conn->privateData; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - vmwareDriverLock(driver); *version = driver->version; - vmwareDriverUnlock(driver); return 0; } @@ -395,6 +378,7 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla vmwareDomainPtr pDomain = NULL; virVMXContext ctx; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -406,7 +390,6 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; - vmwareDriverLock(driver); if ((vmdef = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; @@ -451,7 +434,6 @@ vmwareDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla VIR_FREE(vmxPath); if (vm) virObjectUnlock(vm); - vmwareDriverUnlock(driver); return dom; } @@ -468,11 +450,10 @@ vmwareDomainShutdownFlags(virDomainPtr dom, struct vmware_driver *driver = dom->conn->privateData; virDomainObj *vm; int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); virCheckFlags(0, -1); - vmwareDriverLock(driver); - if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; @@ -494,7 +475,6 @@ vmwareDomainShutdownFlags(virDomainPtr dom, ret = 0; cleanup: virDomainObjEndAPI(&vm); - vmwareDriverUnlock(driver); return ret; } @@ -646,6 +626,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, vmwareDomainPtr pDomain = NULL; virVMXContext ctx; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); virCheckFlags(VIR_DOMAIN_START_VALIDATE, NULL); @@ -657,8 +638,6 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, ctx.autodetectSCSIControllerModel = NULL; ctx.datacenterPath = NULL; - vmwareDriverLock(driver); - if ((vmdef = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; @@ -704,7 +683,6 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, VIR_FREE(vmx); VIR_FREE(vmxPath); virDomainObjEndAPI(&vm); - vmwareDriverUnlock(driver); return dom; } @@ -715,10 +693,10 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, struct vmware_driver *driver = dom->conn->privateData; virDomainObj *vm; int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); virCheckFlags(0, -1); - vmwareDriverLock(driver); if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; @@ -735,7 +713,6 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - vmwareDriverUnlock(driver); return ret; } @@ -752,10 +729,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom, struct vmware_driver *driver = dom->conn->privateData; virDomainObj *vm; int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); virCheckFlags(0, -1); - vmwareDriverLock(driver); if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; @@ -777,7 +754,6 @@ vmwareDomainUndefineFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - vmwareDriverUnlock(driver); return ret; } @@ -791,12 +767,12 @@ static virDomainPtr vmwareDomainLookupByID(virConnectPtr conn, int id) { struct vmware_driver *driver = conn->privateData; - virDomainObj *vm; + virDomainObj *vm = NULL; virDomainPtr dom = NULL; - vmwareDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); - vmwareDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vm = virDomainObjListFindByID(driver->domains, id); + } if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -848,12 +824,12 @@ static virDomainPtr vmwareDomainLookupByName(virConnectPtr conn, const char *name) { struct vmware_driver *driver = conn->privateData; - virDomainObj *vm; + virDomainObj *vm = NULL; virDomainPtr dom = NULL; - vmwareDriverLock(driver); - vm = virDomainObjListFindByName(driver->domains, name); - vmwareDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vm = virDomainObjListFindByName(driver->domains, name); + } if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -972,28 +948,20 @@ static int vmwareConnectNumOfDefinedDomains(virConnectPtr conn) { struct vmware_driver *driver = conn->privateData; - int n; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - vmwareDriverLock(driver); vmwareDomainObjListUpdateAll(driver->domains, driver); - n = virDomainObjListNumOfDomains(driver->domains, false, NULL, NULL); - vmwareDriverUnlock(driver); - - return n; + return virDomainObjListNumOfDomains(driver->domains, false, NULL, NULL); } static int vmwareConnectNumOfDomains(virConnectPtr conn) { struct vmware_driver *driver = conn->privateData; - int n; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - vmwareDriverLock(driver); vmwareDomainObjListUpdateAll(driver->domains, driver); - n = virDomainObjListNumOfDomains(driver->domains, true, NULL, NULL); - vmwareDriverUnlock(driver); - - return n; + return virDomainObjListNumOfDomains(driver->domains, true, NULL, NULL); } @@ -1001,14 +969,10 @@ static int vmwareConnectListDomains(virConnectPtr conn, int *ids, int nids) { struct vmware_driver *driver = conn->privateData; - int n; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - vmwareDriverLock(driver); vmwareDomainObjListUpdateAll(driver->domains, driver); - n = virDomainObjListGetActiveIDs(driver->domains, ids, nids, NULL, NULL); - vmwareDriverUnlock(driver); - - return n; + return virDomainObjListGetActiveIDs(driver->domains, ids, nids, NULL, NULL); } static int @@ -1016,14 +980,11 @@ vmwareConnectListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { struct vmware_driver *driver = conn->privateData; - int n; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - vmwareDriverLock(driver); vmwareDomainObjListUpdateAll(driver->domains, driver); - n = virDomainObjListGetInactiveNames(driver->domains, names, nnames, - NULL, NULL); - vmwareDriverUnlock(driver); - return n; + return virDomainObjListGetInactiveNames(driver->domains, names, nnames, + NULL, NULL); } static int @@ -1093,11 +1054,12 @@ vmwareConnectListAllDomains(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ALL, -1); - vmwareDriverLock(driver); - vmwareDomainObjListUpdateAll(driver->domains, driver); - ret = virDomainObjListExport(driver->domains, conn, domains, - NULL, flags); - vmwareDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vmwareDomainObjListUpdateAll(driver->domains, driver); + ret = virDomainObjListExport(driver->domains, conn, domains, + NULL, flags); + } + return ret; } -- 2.31.1
If the mutex is part of the `driver` object, it cannot guard that object's creation and destruction perfectly. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/secret/secret_driver.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 0220f394ef..d0e819809b 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -52,9 +52,10 @@ enum { SECRET_MAX_XML_FILE = 10*1024*1024 }; /* Internal driver state */ +static virMutex mutex = VIR_MUTEX_INITIALIZER; + typedef struct _virSecretDriverState virSecretDriverState; struct _virSecretDriverState { - virMutex lock; bool privileged; /* readonly */ char *embeddedRoot; /* readonly */ int embeddedRefs; @@ -74,14 +75,14 @@ static virSecretDriverState *driver; static void secretDriverLock(void) { - virMutexLock(&driver->lock); + virMutexLock(&mutex); } static void secretDriverUnlock(void) { - virMutexUnlock(&driver->lock); + virMutexUnlock(&mutex); } @@ -463,7 +464,6 @@ secretStateCleanup(void) VIR_FREE(driver->stateDir); secretDriverUnlock(); - virMutexDestroy(&driver->lock); VIR_FREE(driver); return 0; @@ -479,10 +479,6 @@ secretStateInitialize(bool privileged, driver = g_new0(virSecretDriverState, 1); driver->lockFD = -1; - if (virMutexInit(&driver->lock) < 0) { - VIR_FREE(driver); - return VIR_DRV_STATE_INIT_ERROR; - } secretDriverLock(); driver->secretEventState = virObjectEventStateNew(); -- 2.31.1
On Fri, Feb 11, 2022 at 11:30:39AM +0100, Tim Wiederhake wrote:
If the mutex is part of the `driver` object, it cannot guard that object's creation and destruction perfectly.
The mutex doesn't need to guard the object's creation/destruction in its entirity though. The driver creation/destruction is a onetime thing at startup and shutdown of the daemon. There is a requirement that API calls have ceased before destruction begins, and if that's not the case then the code is unsafe no matter what because it will be liable to access a NULL driver object after acquiring the mutex. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/secret/secret_driver.c | 54 +++++++++++++++----------------------- 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index d0e819809b..09782b38d3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -72,20 +72,6 @@ struct _virSecretDriverState { static virSecretDriverState *driver; -static void -secretDriverLock(void) -{ - virMutexLock(&mutex); -} - - -static void -secretDriverUnlock(void) -{ - virMutexUnlock(&mutex); -} - - static virSecretObj * secretObjFromSecret(virSecretPtr secret) { @@ -447,13 +433,11 @@ secretUndefine(virSecretPtr secret) static int -secretStateCleanup(void) +secretStateCleanupLocked(void) { if (!driver) return -1; - secretDriverLock(); - virObjectUnref(driver->secrets); VIR_FREE(driver->configDir); @@ -463,12 +447,19 @@ secretStateCleanup(void) virPidFileRelease(driver->stateDir, "driver", driver->lockFD); VIR_FREE(driver->stateDir); - secretDriverUnlock(); VIR_FREE(driver); return 0; } +static int +secretStateCleanup(void) +{ + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + + return secretStateCleanupLocked(); +} + static int secretStateInitialize(bool privileged, @@ -476,11 +467,11 @@ secretStateInitialize(bool privileged, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + driver = g_new0(virSecretDriverState, 1); driver->lockFD = -1; - secretDriverLock(); - driver->secretEventState = virObjectEventStateNew(); driver->privileged = privileged; @@ -524,12 +515,10 @@ secretStateInitialize(bool privileged, if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) goto error; - secretDriverUnlock(); return VIR_DRV_STATE_INIT_COMPLETE; error: - secretDriverUnlock(); - secretStateCleanup(); + secretStateCleanupLocked(); return VIR_DRV_STATE_INIT_ERROR; } @@ -537,14 +526,13 @@ secretStateInitialize(bool privileged, static int secretStateReload(void) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + if (!driver) return -1; - secretDriverLock(); - ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); - secretDriverUnlock(); return 0; } @@ -592,11 +580,11 @@ secretConnectOpen(virConnectPtr conn, return VIR_DRV_OPEN_ERROR; if (driver->embeddedRoot) { - secretDriverLock(); - if (driver->embeddedRefs == 0) - virSetConnectSecret(conn); - driver->embeddedRefs++; - secretDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + if (driver->embeddedRefs == 0) + virSetConnectSecret(conn); + driver->embeddedRefs++; + } } return VIR_DRV_OPEN_SUCCESS; @@ -604,12 +592,12 @@ secretConnectOpen(virConnectPtr conn, static int secretConnectClose(virConnectPtr conn G_GNUC_UNUSED) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); + if (driver->embeddedRoot) { - secretDriverLock(); driver->embeddedRefs--; if (driver->embeddedRefs == 0) virSetConnectSecret(NULL); - secretDriverUnlock(); } return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virlockspace.c | 106 +++++++++++++--------------------------- 1 file changed, 33 insertions(+), 73 deletions(-) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index a7f1c2324f..79925053bf 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -414,12 +414,11 @@ virJSONValue *virLockSpacePreExecRestart(virLockSpace *lockspace) g_autoptr(virJSONValue) resources = virJSONValueNewArray(); g_autofree virHashKeyValuePair *pairs = NULL; virHashKeyValuePair *tmp; - - virMutexLock(&lockspace->lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&lockspace->lock); if (lockspace->dir && virJSONValueObjectAppendString(object, "directory", lockspace->dir) < 0) - goto error; + return NULL; tmp = pairs = virHashGetItems(lockspace->resources, NULL, false); @@ -434,41 +433,36 @@ virJSONValue *virLockSpacePreExecRestart(virLockSpace *lockspace) virJSONValueObjectAppendNumberInt(child, "fd", res->fd) < 0 || virJSONValueObjectAppendBoolean(child, "lockHeld", res->lockHeld) < 0 || virJSONValueObjectAppendNumberUint(child, "flags", res->flags) < 0) - goto error; + return NULL; if (virSetInherit(res->fd, true) < 0) { virReportSystemError(errno, "%s", _("Cannot disable close-on-exec flag")); - goto error; + return NULL; } for (i = 0; i < res->nOwners; i++) { g_autoptr(virJSONValue) owner = virJSONValueNewNumberUlong(res->owners[i]); if (!owner) - goto error; + return NULL; if (virJSONValueArrayAppend(owners, &owner) < 0) - goto error; + return NULL; } if (virJSONValueObjectAppend(child, "owners", &owners) < 0) - goto error; + return NULL; if (virJSONValueArrayAppend(resources, &child) < 0) - goto error; + return NULL; tmp++; } if (virJSONValueObjectAppend(object, "resources", &resources) < 0) - goto error; + return NULL; - virMutexUnlock(&lockspace->lock); return g_steal_pointer(&object); - - error: - virMutexUnlock(&lockspace->lock); - return NULL; } @@ -493,67 +487,55 @@ const char *virLockSpaceGetDirectory(virLockSpace *lockspace) int virLockSpaceCreateResource(virLockSpace *lockspace, const char *resname) { - int ret = -1; g_autofree char *respath = NULL; + VIR_LOCK_GUARD lock = virLockGuardLock(&lockspace->lock); VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname); - virMutexLock(&lockspace->lock); - if (virHashLookup(lockspace->resources, resname) != NULL) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), resname); - goto cleanup; + return -1; } if (!(respath = virLockSpaceGetResourcePath(lockspace, resname))) - goto cleanup; + return -1; if (virFileTouch(respath, 0600) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virMutexUnlock(&lockspace->lock); - return ret; + return 0; } int virLockSpaceDeleteResource(virLockSpace *lockspace, const char *resname) { - int ret = -1; g_autofree char *respath = NULL; + VIR_LOCK_GUARD lock = virLockGuardLock(&lockspace->lock); VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname); - virMutexLock(&lockspace->lock); - if (virHashLookup(lockspace->resources, resname) != NULL) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), resname); - goto cleanup; + return -1; } if (!(respath = virLockSpaceGetResourcePath(lockspace, resname))) - goto cleanup; + return -1; if (unlink(respath) < 0 && errno != ENOENT) { virReportSystemError(errno, _("Unable to delete lockspace resource %s"), respath); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virMutexUnlock(&lockspace->lock); - return ret; + return 0; } @@ -562,8 +544,8 @@ int virLockSpaceAcquireResource(virLockSpace *lockspace, pid_t owner, unsigned int flags) { - int ret = -1; virLockSpaceResource *res; + VIR_LOCK_GUARD lock = virLockGuardLock(&lockspace->lock); VIR_DEBUG("lockspace=%p resname=%s flags=0x%x owner=%lld", lockspace, resname, flags, (unsigned long long)owner); @@ -571,8 +553,6 @@ int virLockSpaceAcquireResource(virLockSpace *lockspace, virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1); - virMutexLock(&lockspace->lock); - if ((res = virHashLookup(lockspace->resources, resname))) { if ((res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) && (flags & VIR_LOCK_SPACE_ACQUIRE_SHARED)) { @@ -580,28 +560,23 @@ int virLockSpaceAcquireResource(virLockSpace *lockspace, VIR_EXPAND_N(res->owners, res->nOwners, 1); res->owners[res->nOwners-1] = owner; - goto done; + return 0; } virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), resname); - goto cleanup; + return -1; } if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) - goto cleanup; + return -1; if (virHashAddEntry(lockspace->resources, resname, res) < 0) { virLockSpaceResourceFree(res); - goto cleanup; + return -1; } - done: - ret = 0; - - cleanup: - virMutexUnlock(&lockspace->lock); - return ret; + return 0; } @@ -609,20 +584,18 @@ int virLockSpaceReleaseResource(virLockSpace *lockspace, const char *resname, pid_t owner) { - int ret = -1; virLockSpaceResource *res; size_t i; + VIR_LOCK_GUARD lock = virLockGuardLock(&lockspace->lock); VIR_DEBUG("lockspace=%p resname=%s owner=%lld", lockspace, resname, (unsigned long long)owner); - virMutexLock(&lockspace->lock); - if (!(res = virHashLookup(lockspace->resources, resname))) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is not locked"), resname); - goto cleanup; + return -1; } for (i = 0; i < res->nOwners; i++) { @@ -634,20 +607,16 @@ int virLockSpaceReleaseResource(virLockSpace *lockspace, virReportError(VIR_ERR_INTERNAL_ERROR, _("owner %lld does not hold the resource lock"), (unsigned long long)owner); - goto cleanup; + return -1; } VIR_DELETE_ELEMENT(res->owners, i, res->nOwners); if ((res->nOwners == 0) && virHashRemoveEntry(lockspace->resources, resname) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virMutexUnlock(&lockspace->lock); - return ret; + return 0; } @@ -693,26 +662,17 @@ virLockSpaceRemoveResourcesForOwner(const void *payload, int virLockSpaceReleaseResourcesForOwner(virLockSpace *lockspace, pid_t owner) { - int ret = 0; struct virLockSpaceRemoveData data = { owner, 0 }; + VIR_LOCK_GUARD lock = virLockGuardLock(&lockspace->lock); VIR_DEBUG("lockspace=%p owner=%lld", lockspace, (unsigned long long)owner); - virMutexLock(&lockspace->lock); - if (virHashRemoveSet(lockspace->resources, virLockSpaceRemoveResourcesForOwner, &data) < 0) - goto error; + return -1; - ret = data.count; - - virMutexUnlock(&lockspace->lock); - return ret; - - error: - virMutexUnlock(&lockspace->lock); - return -1; + return data.count; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virtpm.c | 39 ++++++++++++--------------------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/src/util/virtpm.c b/src/util/virtpm.c index c02b42f948..ca7506d82e 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -137,18 +137,12 @@ static int virTPMEmulatorInit(bool quiet); static char * virTPMBinaryGetPath(virTPMBinary binary) { - char *s = NULL; - - virMutexLock(&swtpm_tools_lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&swtpm_tools_lock); if (virTPMEmulatorInit(false) < 0) - goto cleanup; - - s = g_strdup(swtpmBinaries[binary].path); + return NULL; - cleanup: - virMutexUnlock(&swtpm_tools_lock); - return s; + return g_strdup(swtpmBinaries[binary].path); } char * @@ -171,20 +165,14 @@ virTPMGetSwtpmIoctl(void) bool virTPMHasSwtpm(void) { - bool ret = false; - - virMutexLock(&swtpm_tools_lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&swtpm_tools_lock); if (virTPMEmulatorInit(true) < 0) - goto cleanup; + return false; - ret = swtpmBinaries[VIR_TPM_BINARY_SWTPM].path != NULL && + return swtpmBinaries[VIR_TPM_BINARY_SWTPM].path != NULL && swtpmBinaries[VIR_TPM_BINARY_SWTPM_SETUP].path != NULL && swtpmBinaries[VIR_TPM_BINARY_SWTPM_IOCTL].path != NULL; - - cleanup: - virMutexUnlock(&swtpm_tools_lock); - return ret; } /* virTPMExecGetCaps @@ -341,12 +329,10 @@ static bool virTPMBinaryGetCaps(virTPMBinary binary, unsigned int cap) { - bool ret = false; - - virMutexLock(&swtpm_tools_lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&swtpm_tools_lock); if (virTPMEmulatorInit(false) < 0) - goto cleanup; + return false; if (!swtpmBinaries[binary].caps && swtpmBinaries[binary].capsParse) { @@ -355,12 +341,11 @@ virTPMBinaryGetCaps(virTPMBinary binary, swtpmBinaries[binary].path, swtpmBinaries[binary].parm); } - if (swtpmBinaries[binary].caps) - ret = virBitmapIsBitSet(swtpmBinaries[binary].caps, cap); - cleanup: - virMutexUnlock(&swtpm_tools_lock); - return ret; + if (!swtpmBinaries[binary].caps) + return false; + + return virBitmapIsBitSet(swtpmBinaries[binary].caps, cap); } bool -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/vbox/vbox_common.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 40180b0dfd..36db6e06be 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -224,7 +224,7 @@ vboxSdkUninitialize(void) static struct _vboxDriver * vboxGetDriverConnection(void) { - virMutexLock(&vbox_driver_lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&vbox_driver_lock); if (vbox_driver) { virObjectRef(vbox_driver); @@ -234,9 +234,6 @@ vboxGetDriverConnection(void) if (!vbox_driver) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to create vbox driver object.")); - - virMutexUnlock(&vbox_driver_lock); - return NULL; } } @@ -248,26 +245,20 @@ vboxGetDriverConnection(void) virObjectUnref(vbox_driver); if (vboxDriverDisposed) vbox_driver = NULL; - - virMutexUnlock(&vbox_driver_lock); - return NULL; } vbox_driver->connectionCount++; - - virMutexUnlock(&vbox_driver_lock); - return vbox_driver; } static void vboxDestroyDriverConnection(void) { - virMutexLock(&vbox_driver_lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&vbox_driver_lock); if (!vbox_driver) - goto cleanup; + return; vbox_driver->connectionCount--; @@ -277,9 +268,6 @@ vboxDestroyDriverConnection(void) virObjectUnref(vbox_driver); if (vboxDriverDisposed) vbox_driver = NULL; - - cleanup: - virMutexUnlock(&vbox_driver_lock); } static int openSessionForMachine(struct _vboxDriver *data, const unsigned char *dom_uuid, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virfirewall.c | 13 ++++--------- tools/virsh.c | 12 ++++++------ tools/virt-admin.c | 12 ++++++------ tools/vsh.c | 8 ++++---- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 70092f2ef6..31a8352d4e 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -613,9 +613,7 @@ int virFirewallApply(virFirewall *firewall) { size_t i, j; - int ret = -1; - - virMutexLock(&ruleLock); + VIR_LOCK_GUARD lock = virLockGuardLock(&ruleLock); if (!firewall || firewall->err) { int err = EINVAL; @@ -624,7 +622,7 @@ virFirewallApply(virFirewall *firewall) err = firewall->err; virReportSystemError(err, "%s", _("Unable to create rule")); - goto cleanup; + return -1; } VIR_DEBUG("Applying groups for %p", firewall); @@ -657,13 +655,10 @@ virFirewallApply(virFirewall *firewall) virErrorRestore(&saved_error); VIR_DEBUG("Done rolling back groups for %p", firewall); - goto cleanup; + return -1; } } VIR_DEBUG("Done applying groups for %p", firewall); - ret = 0; - cleanup: - virMutexUnlock(&ruleLock); - return ret; + return 0; } diff --git a/tools/virsh.c b/tools/virsh.c index 1c75a66fcb..64e0700fcd 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -412,13 +412,13 @@ virshDeinit(vshControl *ctl) virResetLastError(); if (ctl->eventLoopStarted) { - int timer; + int timer = -1; - virMutexLock(&ctl->lock); - ctl->quit = true; - /* HACK: Add a dummy timeout to break event loop */ - timer = virEventAddTimeout(0, virshDeinitTimer, NULL, NULL); - virMutexUnlock(&ctl->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&ctl->lock) { + ctl->quit = true; + /* HACK: Add a dummy timeout to break event loop */ + timer = virEventAddTimeout(0, virshDeinitTimer, NULL, NULL); + } virThreadJoin(&ctl->eventLoop); diff --git a/tools/virt-admin.c b/tools/virt-admin.c index c0818e850a..e010763e21 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1189,13 +1189,13 @@ vshAdmDeinit(vshControl *ctl) virResetLastError(); if (ctl->eventLoopStarted) { - int timer; + int timer = -1; - virMutexLock(&ctl->lock); - ctl->quit = true; - /* HACK: Add a dummy timeout to break event loop */ - timer = virEventAddTimeout(0, vshAdmDeinitTimer, NULL, NULL); - virMutexUnlock(&ctl->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&ctl->lock) { + ctl->quit = true; + /* HACK: Add a dummy timeout to break event loop */ + timer = virEventAddTimeout(0, vshAdmDeinitTimer, NULL, NULL); + } virThreadJoin(&ctl->eventLoop); diff --git a/tools/vsh.c b/tools/vsh.c index 5056d7e19d..4ec5e54b5d 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2018,10 +2018,10 @@ vshEventLoop(void *opaque) vshControl *ctl = opaque; while (1) { - bool quit; - virMutexLock(&ctl->lock); - quit = ctl->quit; - virMutexUnlock(&ctl->lock); + bool quit = false; + VIR_WITH_MUTEX_LOCK_GUARD(&ctl->lock) { + quit = ctl->quit; + } if (quit) break; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/qemusecuritymock.c | 88 +++++++++++++--------------------------- 1 file changed, 29 insertions(+), 59 deletions(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index 778b0561ac..03c818d8a3 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -137,27 +137,21 @@ virFileGetXAttrQuiet(const char *path, const char *name, char **value) { - int ret = -1; - g_autofree char *key = NULL; + g_autofree char *key = get_key(path, name); char *val; + VIR_LOCK_GUARD lock = virLockGuardLock(&m); - key = get_key(path, name); - - virMutexLock(&m); init_syms(); init_hash(); if (!(val = virHashLookup(xattr_paths, key))) { errno = ENODATA; - goto cleanup; + return -1; } *value = g_strdup(val); - ret = 0; - cleanup: - virMutexUnlock(&m); - return ret; + return 0; } @@ -193,25 +187,18 @@ int virFileSetXAttr(const char *path, const char *name, const char *value) { - int ret = -1; - g_autofree char *key = NULL; - g_autofree char *val = NULL; - - key = get_key(path, name); - val = g_strdup(value); + g_autofree char *key = get_key(path, name); + g_autofree char *val = g_strdup(value); + VIR_LOCK_GUARD lock = virLockGuardLock(&m); - virMutexLock(&m); init_syms(); init_hash(); if (virHashUpdateEntry(xattr_paths, key, val) < 0) - goto cleanup; + return -1; val = NULL; - ret = 0; - cleanup: - virMutexUnlock(&m); - return ret; + return 0; } @@ -219,18 +206,15 @@ int virFileRemoveXAttr(const char *path, const char *name) { int ret = -1; - g_autofree char *key = NULL; - - key = get_key(path, name); + g_autofree char *key = get_key(path, name); + VIR_LOCK_GUARD lock = virLockGuardLock(&m); - virMutexLock(&m); init_syms(); init_hash(); if ((ret = virHashRemoveEntry(xattr_paths, key)) < 0) errno = ENODATA; - virMutexUnlock(&m); return ret; } @@ -270,8 +254,8 @@ mock_chown(const char *path, uid_t uid, gid_t gid) { - uint32_t *val = NULL; - int ret = -1; + g_autofree uint32_t *val = NULL; + VIR_LOCK_GUARD lock = virLockGuardLock(&m); if (gid >> 16 || uid >> 16) { fprintf(stderr, "Attempt to set too high UID or GID: %llu %llu", @@ -283,18 +267,13 @@ mock_chown(const char *path, *val = (gid << 16) + uid; - virMutexLock(&m); init_hash(); if (virHashUpdateEntry(chown_paths, path, val) < 0) - goto cleanup; - val = NULL; + return -1; - ret = 0; - cleanup: - virMutexUnlock(&m); - VIR_FREE(val); - return ret; + val = NULL; + return 0; } @@ -460,13 +439,12 @@ printXATTR(void *payload, */ int checkPaths(GHashTable *paths) { - int ret = -1; checkOwnerData data = { .paths = paths, .chown_fail = false, .selinux_fail = false }; bool xattr_fail = false; GHashTableIter htitr; void *key; + VIR_LOCK_GUARD lock = virLockGuardLock(&m); - virMutexLock(&m); init_hash(); g_hash_table_iter_init(&htitr, paths); @@ -474,38 +452,35 @@ int checkPaths(GHashTable *paths) while (g_hash_table_iter_next(&htitr, &key, NULL)) { if (!virHashLookup(chown_paths, key)) { fprintf(stderr, "Unexpected path restored: %s\n", (const char *) key); - goto cleanup; + return -1; } } if (virHashForEach(selinux_paths, checkSELinux, &data) < 0) - goto cleanup; + return -1; if (virHashForEach(chown_paths, checkOwner, &data) < 0) - goto cleanup; + return -1; if (virHashForEach(xattr_paths, printXATTR, &xattr_fail) < 0) - goto cleanup; + return -1; if (data.chown_fail || data.selinux_fail || xattr_fail) - goto cleanup; + return -1; - ret = 0; - cleanup: - virMutexUnlock(&m); - return ret; + return 0; } void freePaths(void) { - virMutexLock(&m); + VIR_LOCK_GUARD lock = virLockGuardLock(&m); + init_hash(); g_clear_pointer(&selinux_paths, g_hash_table_unref); g_clear_pointer(&chown_paths, g_hash_table_unref); g_clear_pointer(&xattr_paths, g_hash_table_unref); - virMutexUnlock(&m); } @@ -578,19 +553,15 @@ mock_setfilecon_raw(const char *path, const char *context) { g_autofree char *val = g_strdup(context); - int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&m); - virMutexLock(&m); init_hash(); if (virHashUpdateEntry(selinux_paths, path, val) < 0) - goto cleanup; + return -1; val = NULL; - ret = 0; - cleanup: - virMutexUnlock(&m); - return ret; + return 0; } @@ -599,8 +570,8 @@ mock_getfilecon_raw(const char *path, char **context) { const char *val; + VIR_LOCK_GUARD lock = virLockGuardLock(&m); - virMutexLock(&m); init_hash(); val = virHashLookup(selinux_paths, path); @@ -608,7 +579,6 @@ mock_getfilecon_raw(const char *path, val = DEFAULT_SELINUX_LABEL; *context = g_strdup(val); - virMutexUnlock(&m); return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/qemumonitortestutils.c | 65 ++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 37 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index ce8e6e1645..86300da68a 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -253,12 +253,11 @@ qemuMonitorTestIO(virNetSocket *sock, { qemuMonitorTest *test = opaque; bool err = false; + VIR_LOCK_GUARD lock = virLockGuardLock(&test->lock); - virMutexLock(&test->lock); - if (test->quit) { - virMutexUnlock(&test->lock); + if (test->quit) return; - } + if (events & VIR_EVENT_HANDLE_WRITABLE) { ssize_t ret; if ((ret = virNetSocketWrite(sock, @@ -336,7 +335,6 @@ qemuMonitorTestIO(virNetSocket *sock, virNetSocketUpdateIOCallback(sock, events); } - virMutexUnlock(&test->lock); } @@ -345,24 +343,22 @@ qemuMonitorTestWorker(void *opaque) { qemuMonitorTest *test = opaque; - virMutexLock(&test->lock); - - while (!test->quit) { - virMutexUnlock(&test->lock); + while (true) { + VIR_WITH_MUTEX_LOCK_GUARD(&test->lock) { + if (test->quit) { + test->running = false; + return; + } + } if (virEventRunDefaultImpl() < 0) { - virMutexLock(&test->lock); - test->quit = true; - break; + VIR_WITH_MUTEX_LOCK_GUARD(&test->lock) { + test->quit = true; + test->running = false; + return; + } } - - virMutexLock(&test->lock); } - - test->running = false; - - virMutexUnlock(&test->lock); - return; } @@ -383,13 +379,13 @@ qemuMonitorTestFree(qemuMonitorTest *test) if (!test) return; - virMutexLock(&test->lock); - if (test->running) { - test->quit = true; - /* HACK: Add a dummy timeout to break event loop */ - timer = virEventAddTimeout(0, qemuMonitorTestFreeTimer, NULL, NULL); + VIR_WITH_MUTEX_LOCK_GUARD(&test->lock) { + if (test->running) { + test->quit = true; + /* HACK: Add a dummy timeout to break event loop */ + timer = virEventAddTimeout(0, qemuMonitorTestFreeTimer, NULL, NULL); + } } - virMutexUnlock(&test->lock); if (test->client) { virNetSocketRemoveIOCallback(test->client); @@ -463,9 +459,9 @@ qemuMonitorTestAddHandler(qemuMonitorTest *test, item->freecb = freecb; item->opaque = opaque; - virMutexLock(&test->lock); - VIR_APPEND_ELEMENT(test->items, test->nitems, item); - virMutexUnlock(&test->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&test->lock) { + VIR_APPEND_ELEMENT(test->items, test->nitems, item); + } return 0; } @@ -1072,16 +1068,11 @@ qemuMonitorCommonTestInit(qemuMonitorTest *test) NULL) < 0) return -1; - virMutexLock(&test->lock); - if (virThreadCreate(&test->thread, - true, - qemuMonitorTestWorker, - test) < 0) { - virMutexUnlock(&test->lock); - return -1; + VIR_WITH_MUTEX_LOCK_GUARD(&test->lock) { + if (virThreadCreate(&test->thread, true, qemuMonitorTestWorker, test) < 0) + return -1; + test->started = test->running = true; } - test->started = test->running = true; - virMutexUnlock(&test->lock); return 0; } -- 2.31.1
On 2/11/22 11:30, Tim Wiederhake wrote:
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management.
Tim Wiederhake (10): vz: Use automatic mutex management vmware: Use automatic mutex management secret: Factor out mutex secret: Use automatic mutex management virlockspace: Use automatic mutex management virtpm: Use automatic mutex management vbox: Use automatic mutex management tools: Use automatic mutex management qemusecuritymock: Use automatic mutex management qemumonitortestutils: Use automatic mutex management
src/secret/secret_driver.c | 62 ++++++++------------ src/util/virfirewall.c | 13 ++--- src/util/virlockspace.c | 106 +++++++++++------------------------ src/util/virtpm.c | 39 ++++--------- src/vbox/vbox_common.c | 18 +----- src/vmware/vmware_driver.c | 100 ++++++++++----------------------- src/vz/vz_driver.c | 44 +++++++-------- tests/qemumonitortestutils.c | 65 +++++++++------------ tests/qemusecuritymock.c | 88 ++++++++++------------------- tools/virsh.c | 12 ++-- tools/virt-admin.c | 12 ++-- tools/vsh.c | 8 +-- 12 files changed, 201 insertions(+), 366 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Daniel P. Berrangé -
Michal Prívozník -
Tim Wiederhake