On 2/7/22 14:12, Tim Wiederhake wrote:
> This leaves a bogus `virMutexUnlock` in `chDomainCreateXML`.
>
> Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx>
> ---
> src/ch/ch_conf.c | 20 ++++++++------------
> src/ch/ch_conf.h | 10 ----------
> src/ch/ch_driver.c | 30 +++++++++++++++---------------
> 3 files changed, 23 insertions(+), 37 deletions(-)
>
> diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
> index cdf69e3e70..ccc01ecdab 100644
> --- a/src/ch/ch_conf.c
> +++ b/src/ch/ch_conf.c
> @@ -93,16 +93,15 @@ virCaps *virCHDriverGetCapabilities(virCHDriver *driver,
> if (refresh && !(caps = virCHDriverCapsInit()))
> return NULL;
>
> - chDriverLock(driver);
> + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) {
> + if (refresh) {
> + virObjectUnref(driver->caps);
> + driver->caps = caps;
> + }
>
> - if (refresh) {
> - virObjectUnref(driver->caps);
> - driver->caps = caps;
> + ret = virObjectRef(driver->caps);
> }
>
> - ret = virObjectRef(driver->caps);
> - chDriverUnlock(driver);
> -
> return ret;
> }
>
> @@ -159,11 +158,8 @@ virCHDriverConfigNew(bool privileged)
>
> virCHDriverConfig *virCHDriverGetConfig(virCHDriver *driver)
> {
> - virCHDriverConfig *cfg;
> - chDriverLock(driver);
> - cfg = virObjectRef(driver->config);
> - chDriverUnlock(driver);
> - return cfg;
> + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock);
> + return virObjectRef(driver->config);
> }
>
> static void
> diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h
> index c56caa3f5f..b927621a97 100644
> --- a/src/ch/ch_conf.h
> +++ b/src/ch/ch_conf.h
> @@ -78,13 +78,3 @@ virDomainXMLOption *chDomainXMLConfInit(virCHDriver *driver);
> virCHDriverConfig *virCHDriverConfigNew(bool privileged);
> virCHDriverConfig *virCHDriverGetConfig(virCHDriver *driver);
> int chExtractVersion(virCHDriver *driver);
> -
> -static inline void chDriverLock(virCHDriver *driver)
> -{
> - virMutexLock(&driver->lock);
> -}
> -
> -static inline void chDriverUnlock(virCHDriver *driver)
> -{
> - virMutexUnlock(&driver->lock);
> -}
Until here I have no objections. But the code below, I mean the
pre-existing one is total mess and we should clean it up first, because
it will drop some locking.
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 3223f31367..945a1aa225 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -106,9 +106,10 @@ static int chConnectGetVersion(virConnectPtr conn,
> if (virConnectGetVersionEnsureACL(conn) < 0)
> return -1;
>
> - chDriverLock(driver);
> - *version = driver->version;
> - chDriverUnlock(driver);
> + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) {
> + *version = driver->version;
> + }
> +
I see no point in locking the driver here. Firstly, the @version member
is set in chExtractVersion() which is called exactly once from
chStateInitialize() and driver init happens in a single thread (for a
good reason). And even if it didn't, the initialize function never locks
the driver, so this lock here guards nothing.
> return 0;
> }
>
> @@ -241,7 +242,7 @@ chDomainCreateXML(virConnectPtr conn,
> virDomainObjListRemove(driver->domains, vm);
> }
> virDomainObjEndAPI(&vm);
> - chDriverUnlock(driver);
> + virMutexUnlock(&driver->lock);
I'm sorry, but what? I know, not your fault, but this is clearly an
imbalance in mutex locking. Not to mention that this function doesn't
end job properly (typical 'goto error' instead of 'goto endjob' problem).
> return dom;
> }
>
> @@ -368,8 +369,8 @@ static int chDomainIsActive(virDomainPtr dom)
> virCHDriver *driver = dom->conn->privateData;
> virDomainObj *vm;
> int ret = -1;
> + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock);
>
> - chDriverLock(driver);
I'm failing to see the purpose of this locking in the first place.
Again, neither of these problems are your fault, I'm just pointing them out.
> if (!(vm = virCHDomainObjFromDomain(dom)))
> goto cleanup;
>
> @@ -380,7 +381,6 @@ static int chDomainIsActive(virDomainPtr dom)
>
> cleanup:
> virDomainObjEndAPI(&vm);
> - chDriverUnlock(driver);
> return ret;
> }
>
> @@ -638,9 +638,9 @@ static virDomainPtr chDomainLookupByID(virConnectPtr conn,
> virDomainObj *vm;
> virDomainPtr dom = NULL;
>
> - chDriverLock(driver);
> - vm = virDomainObjListFindByID(driver->domains, id);
> - chDriverUnlock(driver);
> + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) {
> + vm = virDomainObjListFindByID(driver->domains, id);
> + }
This is very suboptimal thing to do. I've specifically introduced RW
locks to virDomainObjList so that we don't have to lock the whole driver
when accessing the list.
Alright, let me post patches for these and then get back to you. Patches
until here look good.
Michal
