[libvirt PATCH 00/10] Automatic mutex management - part 6
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management. Tim Wiederhake (10): virstorageobj: Replace deprecated virHash functions virStoragePoolObjAddVol: Simplify error path virStorageVolObjEndAPI: Remove virStorageVolObjNew: Move locking to usage site virstorageobject: Use automatic mutex management virNetServerProcessClients: Remove goto virnetserver: Use automatic mutex management virnetserverclient: Use automatic mutex management security_manager: Use automatic mutex management vz_driver: Use automatic mutex management src/conf/virstorageobj.c | 185 +++++------ src/rpc/virnetserver.c | 295 +++++++----------- src/rpc/virnetserverclient.c | 432 ++++++++++++-------------- src/security/security_manager.c | 524 +++++++++++++------------------- src/vz/vz_driver.c | 50 ++- 5 files changed, 604 insertions(+), 882 deletions(-) -- 2.31.1
Checking for duplicate / NULL keys beforehand will simplify error handling in a later patch significantly. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virstorageobj.c | 47 ++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index b57c1cb8b2..99aee9ffb4 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -521,8 +521,8 @@ virStoragePoolObjRemove(virStoragePoolObjList *pools, virObjectUnlock(obj); virObjectRWLockWrite(pools); virObjectLock(obj); - virHashRemoveEntry(pools->objs, uuidstr); - virHashRemoveEntry(pools->objsName, obj->def->name); + g_hash_table_remove(pools->objs, uuidstr); + g_hash_table_remove(pools->objsName, obj->def->name); virObjectUnref(obj); virObjectRWUnlock(pools); } @@ -621,9 +621,9 @@ virStoragePoolObjClearVols(virStoragePoolObj *obj) if (!obj->volumes) return; - virHashRemoveAll(obj->volumes->objsKey); - virHashRemoveAll(obj->volumes->objsName); - virHashRemoveAll(obj->volumes->objsPath); + g_hash_table_remove_all(obj->volumes->objsKey); + g_hash_table_remove_all(obj->volumes->objsName); + g_hash_table_remove_all(obj->volumes->objsPath); } @@ -639,21 +639,19 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj, if (!(volobj = virStorageVolObjNew())) goto error; - if (virHashAddEntry(volumes->objsKey, voldef->key, volobj) < 0) + if (!voldef->key || !voldef->name || !voldef->target.path || + g_hash_table_contains(volumes->objsKey, voldef->key) || + g_hash_table_contains(volumes->objsName, voldef->name) || + g_hash_table_contains(volumes->objsPath, voldef->target.path)) goto error; + + g_hash_table_insert(volumes->objsKey, g_strdup(voldef->key), volobj); virObjectRef(volobj); - if (virHashAddEntry(volumes->objsName, voldef->name, volobj) < 0) { - virHashRemoveEntry(volumes->objsKey, voldef->key); - goto error; - } + g_hash_table_insert(volumes->objsName, g_strdup(voldef->name), volobj); virObjectRef(volobj); - if (virHashAddEntry(volumes->objsPath, voldef->target.path, volobj) < 0) { - virHashRemoveEntry(volumes->objsKey, voldef->key); - virHashRemoveEntry(volumes->objsName, voldef->name); - goto error; - } + g_hash_table_insert(volumes->objsPath, g_strdup(voldef->target.path), volobj); virObjectRef(volobj); volobj->voldef = voldef; @@ -688,9 +686,9 @@ virStoragePoolObjRemoveVol(virStoragePoolObj *obj, virObjectRef(volobj); virObjectLock(volobj); - virHashRemoveEntry(volumes->objsKey, voldef->key); - virHashRemoveEntry(volumes->objsName, voldef->name); - virHashRemoveEntry(volumes->objsPath, voldef->target.path); + g_hash_table_remove(volumes->objsKey, voldef->key); + g_hash_table_remove(volumes->objsName, voldef->name); + g_hash_table_remove(volumes->objsPath, voldef->target.path); virStorageVolObjEndAPI(&volobj); virObjectRWUnlock(volumes); @@ -1582,15 +1580,18 @@ virStoragePoolObjListAdd(virStoragePoolObjList *pools, goto error; virUUIDFormat((*def)->uuid, uuidstr); - if (virHashAddEntry(pools->objs, uuidstr, obj) < 0) + + if (!(*def)->name || + g_hash_table_contains(pools->objs, uuidstr) || + g_hash_table_contains(pools->objsName, (*def)->name)) goto error; + + g_hash_table_insert(pools->objs, g_strdup(uuidstr), obj); virObjectRef(obj); - if (virHashAddEntry(pools->objsName, (*def)->name, obj) < 0) { - virHashRemoveEntry(pools->objs, uuidstr); - goto error; - } + g_hash_table_insert(pools->objsName, g_strdup((*def)->name), obj); virObjectRef(obj); + obj->def = g_steal_pointer(def); virObjectRWUnlock(pools); return obj; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virstorageobj.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 99aee9ffb4..1ecf35640c 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -636,14 +636,18 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj, virObjectRWLockWrite(volumes); - if (!(volobj = virStorageVolObjNew())) - goto error; - if (!voldef->key || !voldef->name || !voldef->target.path || g_hash_table_contains(volumes->objsKey, voldef->key) || g_hash_table_contains(volumes->objsName, voldef->name) || - g_hash_table_contains(volumes->objsPath, voldef->target.path)) - goto error; + g_hash_table_contains(volumes->objsPath, voldef->target.path)) { + virObjectRWUnlock(volumes); + return -1; + } + + if (!(volobj = virStorageVolObjNew())) { + virObjectRWUnlock(volumes); + return -1; + } g_hash_table_insert(volumes->objsKey, g_strdup(voldef->key), volobj); virObjectRef(volobj); @@ -655,14 +659,10 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj, virObjectRef(volobj); volobj->voldef = voldef; - virObjectRWUnlock(volumes); - virStorageVolObjEndAPI(&volobj); - return 0; - error: virStorageVolObjEndAPI(&volobj); virObjectRWUnlock(volumes); - return -1; + return 0; } -- 2.31.1
This allows a later patch to replace virObjectLock/Unlock pairs with automatic mutex management code. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virstorageobj.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 1ecf35640c..108ae0aeb1 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -138,17 +138,6 @@ virStorageVolObjNew(void) } -static void -virStorageVolObjEndAPI(virStorageVolObj **obj) -{ - if (!*obj) - return; - - virObjectUnlock(*obj); - g_clear_pointer(obj, virObjectUnref); -} - - static void virStorageVolObjDispose(void *opaque) { @@ -660,7 +649,8 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj, volobj->voldef = voldef; - virStorageVolObjEndAPI(&volobj); + virObjectUnlock(volobj); + virObjectUnref(volobj); virObjectRWUnlock(volumes); return 0; } @@ -689,8 +679,8 @@ virStoragePoolObjRemoveVol(virStoragePoolObj *obj, g_hash_table_remove(volumes->objsKey, voldef->key); g_hash_table_remove(volumes->objsName, voldef->name); g_hash_table_remove(volumes->objsPath, voldef->target.path); - virStorageVolObjEndAPI(&volobj); - + virObjectUnlock(volobj); + virObjectUnref(volobj); virObjectRWUnlock(volumes); } -- 2.31.1
This allows a later patch to replace virObjectLock/Unlock pairs with automatic mutex management code. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virstorageobj.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 108ae0aeb1..c837f96ae6 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -125,16 +125,10 @@ VIR_ONCE_GLOBAL_INIT(virStorageVolObj); static virStorageVolObj * virStorageVolObjNew(void) { - virStorageVolObj *obj; - if (virStorageVolObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virStorageVolObjClass))) - return NULL; - - virObjectLock(obj); - return obj; + return virObjectLockableNew(virStorageVolObjClass); } @@ -638,6 +632,8 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj, return -1; } + virObjectLock(volobj); + g_hash_table_insert(volumes->objsKey, g_strdup(voldef->key), volobj); virObjectRef(volobj); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virstorageobj.c | 116 ++++++++++++++------------------------- 1 file changed, 42 insertions(+), 74 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index c837f96ae6..fe868d25fa 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -455,11 +455,11 @@ virStoragePoolObjListSearchCb(const void *payload, virStoragePoolObj *obj = (virStoragePoolObj *) payload; struct _virStoragePoolObjListSearchData *data = (struct _virStoragePoolObjListSearchData *)opaque; + VIR_LOCK_GUARD lock = virObjectLockGuard(obj); - virObjectLock(obj); if (data->searcher(obj, data->opaque)) return 1; - virObjectUnlock(obj); + return 0; } @@ -632,20 +632,19 @@ virStoragePoolObjAddVol(virStoragePoolObj *obj, return -1; } - virObjectLock(volobj); + VIR_WITH_OBJECT_LOCK_GUARD(volobj) { + g_hash_table_insert(volumes->objsKey, g_strdup(voldef->key), volobj); + virObjectRef(volobj); - g_hash_table_insert(volumes->objsKey, g_strdup(voldef->key), volobj); - virObjectRef(volobj); + g_hash_table_insert(volumes->objsName, g_strdup(voldef->name), volobj); + virObjectRef(volobj); - g_hash_table_insert(volumes->objsName, g_strdup(voldef->name), volobj); - virObjectRef(volobj); - - g_hash_table_insert(volumes->objsPath, g_strdup(voldef->target.path), volobj); - virObjectRef(volobj); + g_hash_table_insert(volumes->objsPath, g_strdup(voldef->target.path), volobj); + virObjectRef(volobj); - volobj->voldef = voldef; + volobj->voldef = voldef; + } - virObjectUnlock(volobj); virObjectUnref(volobj); virObjectRWUnlock(volumes); return 0; @@ -671,11 +670,11 @@ virStoragePoolObjRemoveVol(virStoragePoolObj *obj, voldef->name, obj->def->name); virObjectRef(volobj); - virObjectLock(volobj); - g_hash_table_remove(volumes->objsKey, voldef->key); - g_hash_table_remove(volumes->objsName, voldef->name); - g_hash_table_remove(volumes->objsPath, voldef->target.path); - virObjectUnlock(volobj); + VIR_WITH_OBJECT_LOCK_GUARD(volobj) { + g_hash_table_remove(volumes->objsKey, voldef->key); + g_hash_table_remove(volumes->objsName, voldef->name); + g_hash_table_remove(volumes->objsPath, voldef->target.path); + } virObjectUnref(volobj); virObjectRWUnlock(volumes); } @@ -704,16 +703,14 @@ virStoragePoolObjForEachVolumeCb(void *payload, const char *name G_GNUC_UNUSED, void *opaque) { - int ret = 0; virStorageVolObj *volobj = payload; struct _virStoragePoolObjForEachVolData *data = opaque; + VIR_LOCK_GUARD lock = virObjectLockGuard(volobj); - virObjectLock(volobj); if (data->iter(volobj->voldef, data->opaque) < 0) - ret = -1; - virObjectUnlock(volobj); + return -1; - return ret; + return 0; } @@ -746,14 +743,12 @@ virStoragePoolObjSearchVolumeCb(const void *payload, virStorageVolObj *volobj = (virStorageVolObj *) payload; struct _virStoragePoolObjSearchVolData *data = (struct _virStoragePoolObjSearchVolData *) opaque; - int found = 0; + VIR_LOCK_GUARD lock = virObjectLockGuard(volobj); - virObjectLock(volobj); if (data->iter(volobj->voldef, data->opaque)) - found = 1; - virObjectUnlock(volobj); + return 1; - return found; + return 0; } @@ -842,17 +837,12 @@ virStoragePoolObjNumOfVolumesCb(void *payload, { virStorageVolObj *volobj = payload; struct _virStorageVolObjCountData *data = opaque; + VIR_LOCK_GUARD lock = virObjectLockGuard(volobj); - virObjectLock(volobj); - - if (data->filter && - !data->filter(data->conn, data->pooldef, volobj->voldef)) - goto cleanup; + if (data->filter && !data->filter(data->conn, data->pooldef, volobj->voldef)) + return 0; data->count++; - - cleanup: - virObjectUnlock(volobj); return 0; } @@ -891,6 +881,7 @@ virStoragePoolObjVolumeGetNamesCb(void *payload, { virStorageVolObj *volobj = payload; struct _virStorageVolObjNameData *data = opaque; + VIR_LOCK_GUARD lock = virObjectLockGuard(volobj); if (data->error) return 0; @@ -898,19 +889,13 @@ virStoragePoolObjVolumeGetNamesCb(void *payload, if (data->maxnames >= 0 && data->nnames == data->maxnames) return 0; - virObjectLock(volobj); - - if (data->filter && - !data->filter(data->conn, data->pooldef, volobj->voldef)) - goto cleanup; + if (data->filter && !data->filter(data->conn, data->pooldef, volobj->voldef)) + return 0; if (data->names) data->names[data->nnames] = g_strdup(volobj->voldef->name); data->nnames++; - - cleanup: - virObjectUnlock(volobj); return 0; } @@ -961,30 +946,25 @@ virStoragePoolObjVolumeListExportCallback(void *payload, virStorageVolObj *volobj = payload; virStoragePoolObjVolumeListExportData *data = opaque; virStorageVolPtr vol = NULL; + VIR_LOCK_GUARD lock = virObjectLockGuard(volobj); if (data->error) return 0; - virObjectLock(volobj); - - if (data->filter && - !data->filter(data->conn, data->pooldef, volobj->voldef)) - goto cleanup; + if (data->filter && !data->filter(data->conn, data->pooldef, volobj->voldef)) + return 0; if (data->vols) { if (!(vol = virGetStorageVol(data->conn, data->pooldef->name, volobj->voldef->name, volobj->voldef->key, NULL, NULL))) { data->error = true; - goto cleanup; + return 0; } data->vols[data->nvols] = vol; } data->nvols++; - - cleanup: - virObjectUnlock(volobj); return 0; } @@ -1799,19 +1779,15 @@ virStoragePoolObjNumOfStoragePoolsCb(void *payload, { virStoragePoolObj *obj = payload; struct _virStoragePoolCountData *data = opaque; - - virObjectLock(obj); + VIR_LOCK_GUARD lock = virObjectLockGuard(obj); if (data->filter && !data->filter(data->conn, obj->def)) - goto cleanup; + return 0; if (data->wantActive != virStoragePoolObjIsActive(obj)) - goto cleanup; + return 0; data->count++; - - cleanup: - virObjectUnlock(obj); return 0; } @@ -1851,6 +1827,7 @@ virStoragePoolObjGetNamesCb(void *payload, { virStoragePoolObj *obj = payload; struct _virStoragePoolNameData *data = opaque; + VIR_LOCK_GUARD lock = virObjectLockGuard(obj); if (data->error) return 0; @@ -1858,21 +1835,16 @@ virStoragePoolObjGetNamesCb(void *payload, if (data->maxnames >= 0 && data->nnames == data->maxnames) return 0; - virObjectLock(obj); - if (data->filter && !data->filter(data->conn, obj->def)) - goto cleanup; + return 0; if (data->wantActive != virStoragePoolObjIsActive(obj)) - goto cleanup; + return 0; if (data->names) data->names[data->nnames] = g_strdup(obj->def->name); data->nnames++; - - cleanup: - virObjectUnlock(obj); return 0; } @@ -1994,31 +1966,27 @@ virStoragePoolObjListExportCallback(void *payload, virStoragePoolObj *obj = payload; virStoragePoolObjListExportData *data = opaque; virStoragePoolPtr pool = NULL; + VIR_LOCK_GUARD lock = virObjectLockGuard(obj); if (data->error) return 0; - virObjectLock(obj); - if (data->filter && !data->filter(data->conn, obj->def)) - goto cleanup; + return 0; if (!virStoragePoolObjMatch(obj, data->flags)) - goto cleanup; + return 0; if (data->pools) { if (!(pool = virGetStoragePool(data->conn, obj->def->name, obj->def->uuid, NULL, NULL))) { data->error = true; - goto cleanup; + return 0; } data->pools[data->nPools] = pool; } data->nPools++; - - cleanup: - virObjectUnlock(obj); return 0; } -- 2.31.1
This gets rid of the goto and prepares the function for automatic mutex management. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/rpc/virnetserver.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index d9430a2cfa..7824d121cd 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -960,13 +960,19 @@ virNetServerHasClients(virNetServer *srv) void virNetServerProcessClients(virNetServer *srv) { - size_t i; - virNetServerClient *client; + size_t i = 0; - virObjectLock(srv); + while (true) { + virNetServerClient *client; + bool removed = false; + + virObjectLock(srv); + + if (i >= srv->nclients) { + virObjectUnlock(srv); + return; + } - reprocess: - for (i = 0; i < srv->nclients; i++) { client = srv->clients[i]; virObjectLock(client); if (virNetServerClientWantCloseLocked(client)) @@ -974,24 +980,26 @@ virNetServerProcessClients(virNetServer *srv) if (virNetServerClientIsClosedLocked(client)) { VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); + removed = true; /* Update server authentication tracking */ virNetServerSetClientAuthCompletedLocked(srv, client); - virObjectUnlock(client); - virNetServerCheckLimits(srv); + } + + virObjectUnlock(client); + if (removed) { + i = 0; virObjectUnlock(srv); virObjectUnref(client); - virObjectLock(srv); - - goto reprocess; - } else { - virObjectUnlock(client); + continue; } - } - virObjectUnlock(srv); + i++; + + virObjectUnlock(srv); + } } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/rpc/virnetserver.c | 277 +++++++++++++++-------------------------- 1 file changed, 100 insertions(+), 177 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 7824d121cd..9b333f1a6c 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -97,13 +97,9 @@ VIR_ONCE_GLOBAL_INIT(virNetServer); unsigned long long virNetServerNextClientID(virNetServer *srv) { - unsigned long long val; + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - virObjectLock(srv); - val = srv->next_client_id++; - virObjectUnlock(srv); - - return val; + return srv->next_client_id++; } @@ -208,13 +204,13 @@ virNetServerDispatchNewMessage(virNetServerClient *client, VIR_DEBUG("server=%p client=%p message=%p", srv, client, msg); - virObjectLock(srv); - prog = virNetServerGetProgramLocked(srv, msg); - /* we can unlock @srv since @prog can only become invalid in case - * of disposing @srv, but let's grab a ref first to ensure nothing - * disposes of it before we use it. */ - virObjectRef(srv); - virObjectUnlock(srv); + VIR_WITH_OBJECT_LOCK_GUARD(srv) { + prog = virNetServerGetProgramLocked(srv, msg); + /* we can unlock @srv since @prog can only become invalid in case + * of disposing @srv, but let's grab a ref first to ensure nothing + * disposes of it before we use it. */ + virObjectRef(srv); + } if (virThreadPoolGetMaxWorkers(srv->workers) > 0) { virNetServerJob *job; @@ -304,35 +300,28 @@ int virNetServerAddClient(virNetServer *srv, virNetServerClient *client) { - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); if (virNetServerClientInit(client) < 0) - goto error; + return -1; VIR_EXPAND_N(srv->clients, srv->nclients, 1); srv->clients[srv->nclients-1] = virObjectRef(client); - virObjectLock(client); - if (virNetServerClientIsAuthPendingLocked(client)) - virNetServerTrackPendingAuthLocked(srv); - virObjectUnlock(client); + VIR_WITH_OBJECT_LOCK_GUARD(client) { + if (virNetServerClientIsAuthPendingLocked(client)) + virNetServerTrackPendingAuthLocked(srv); + } virNetServerCheckLimits(srv); - virNetServerClientSetDispatcher(client, - virNetServerDispatchNewMessage, - srv); + virNetServerClientSetDispatcher(client, virNetServerDispatchNewMessage, srv); if (virNetServerClientInitKeepAlive(client, srv->keepaliveInterval, srv->keepaliveCount) < 0) - goto error; + return -1; - virObjectUnlock(srv); return 0; - - error: - virObjectUnlock(srv); - return -1; } @@ -565,65 +554,62 @@ virNetServerPreExecRestart(virNetServer *srv) g_autoptr(virJSONValue) clients = virJSONValueNewArray(); g_autoptr(virJSONValue) services = virJSONValueNewArray(); size_t i; - - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); if (virJSONValueObjectAppendNumberUint(object, "min_workers", virThreadPoolGetMinWorkers(srv->workers)) < 0) - goto error; + return NULL; + if (virJSONValueObjectAppendNumberUint(object, "max_workers", virThreadPoolGetMaxWorkers(srv->workers)) < 0) - goto error; + return NULL; + if (virJSONValueObjectAppendNumberUint(object, "priority_workers", virThreadPoolGetPriorityWorkers(srv->workers)) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendNumberUint(object, "max_clients", srv->nclients_max) < 0) - goto error; + return NULL; + if (virJSONValueObjectAppendNumberUint(object, "max_anonymous_clients", srv->nclients_unauth_max) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendNumberUint(object, "keepaliveInterval", srv->keepaliveInterval) < 0) - goto error; + return NULL; + if (virJSONValueObjectAppendNumberUint(object, "keepaliveCount", srv->keepaliveCount) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendNumberUlong(object, "next_client_id", srv->next_client_id) < 0) - goto error; + return NULL; for (i = 0; i < srv->nservices; i++) { g_autoptr(virJSONValue) child = NULL; if (!(child = virNetServerServicePreExecRestart(srv->services[i]))) - goto error; + return NULL; if (virJSONValueArrayAppend(services, &child) < 0) - goto error; + return NULL; } if (virJSONValueObjectAppend(object, "services", &services) < 0) - goto error; + return NULL; for (i = 0; i < srv->nclients; i++) { g_autoptr(virJSONValue) child = NULL; if (!(child = virNetServerClientPreExecRestart(srv->clients[i]))) - goto error; + return NULL; if (virJSONValueArrayAppend(clients, &child) < 0) - goto error; + return NULL; } if (virJSONValueObjectAppend(object, "clients", &clients) < 0) - goto error; - - virObjectUnlock(srv); + return NULL; return g_steal_pointer(&object); - - error: - virObjectUnlock(srv); - return NULL; } @@ -631,16 +617,12 @@ int virNetServerAddService(virNetServer *srv, virNetServerService *svc) { - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); VIR_EXPAND_N(srv->services, srv->nservices, 1); srv->services[srv->nservices-1] = virObjectRef(svc); - virNetServerServiceSetDispatcher(svc, - virNetServerDispatchNewClient, - srv); - - virObjectUnlock(srv); + virNetServerServiceSetDispatcher(svc, virNetServerDispatchNewClient, srv); return 0; } @@ -795,12 +777,10 @@ int virNetServerAddProgram(virNetServer *srv, virNetServerProgram *prog) { - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); VIR_EXPAND_N(srv->programs, srv->nprograms, 1); srv->programs[srv->nprograms-1] = virObjectRef(prog); - - virObjectUnlock(srv); return 0; } @@ -846,13 +826,12 @@ void virNetServerSetClientAuthenticated(virNetServer *srv, virNetServerClient *client) { - virObjectLock(srv); - virObjectLock(client); + VIR_LOCK_GUARD server_lock = virObjectLockGuard(srv); + VIR_LOCK_GUARD client_lock = virObjectLockGuard(srv); + virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); virNetServerSetClientAuthCompletedLocked(srv, client); virNetServerCheckLimits(srv); - virObjectUnlock(client); - virObjectUnlock(srv); } @@ -871,9 +850,9 @@ void virNetServerUpdateServices(virNetServer *srv, bool enabled) { - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); + virNetServerUpdateServicesLocked(srv, enabled); - virObjectUnlock(srv); } @@ -904,22 +883,20 @@ virNetServerDispose(void *obj) void virNetServerClose(virNetServer *srv) { - size_t i; - if (!srv) return; - virObjectLock(srv); + VIR_WITH_OBJECT_LOCK_GUARD(srv) { + size_t i; - for (i = 0; i < srv->nservices; i++) - virNetServerServiceClose(srv->services[i]); - - for (i = 0; i < srv->nclients; i++) - virNetServerClientClose(srv->clients[i]); + for (i = 0; i < srv->nservices; i++) + virNetServerServiceClose(srv->services[i]); - virThreadPoolStop(srv->workers); + for (i = 0; i < srv->nclients; i++) + virNetServerClientClose(srv->clients[i]); - virObjectUnlock(srv); + virThreadPoolStop(srv->workers); + } } @@ -947,13 +924,9 @@ virNetServerTrackCompletedAuthLocked(virNetServer *srv) bool virNetServerHasClients(virNetServer *srv) { - bool ret; - - virObjectLock(srv); - ret = !!srv->nclients; - virObjectUnlock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - return ret; + return !!srv->nclients; } @@ -963,42 +936,37 @@ virNetServerProcessClients(virNetServer *srv) size_t i = 0; while (true) { + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); virNetServerClient *client; bool removed = false; - virObjectLock(srv); - if (i >= srv->nclients) { - virObjectUnlock(srv); return; } client = srv->clients[i]; - virObjectLock(client); - if (virNetServerClientWantCloseLocked(client)) - virNetServerClientCloseLocked(client); - if (virNetServerClientIsClosedLocked(client)) { - VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); - removed = true; + VIR_WITH_OBJECT_LOCK_GUARD(client) { + if (virNetServerClientWantCloseLocked(client)) + virNetServerClientCloseLocked(client); - /* Update server authentication tracking */ - virNetServerSetClientAuthCompletedLocked(srv, client); - virNetServerCheckLimits(srv); - } + if (virNetServerClientIsClosedLocked(client)) { + VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); + removed = true; - virObjectUnlock(client); + /* Update server authentication tracking */ + virNetServerSetClientAuthCompletedLocked(srv, client); + virNetServerCheckLimits(srv); + } + } if (removed) { i = 0; - virObjectUnlock(srv); virObjectUnref(client); continue; } i++; - - virObjectUnlock(srv); } } @@ -1019,7 +987,7 @@ virNetServerGetThreadPoolParameters(virNetServer *srv, size_t *nPrioWorkers, size_t *jobQueueDepth) { - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); *minWorkers = virThreadPoolGetMinWorkers(srv->workers); *maxWorkers = virThreadPoolGetMaxWorkers(srv->workers); @@ -1028,7 +996,6 @@ virNetServerGetThreadPoolParameters(virNetServer *srv, *nPrioWorkers = virThreadPoolGetPriorityWorkers(srv->workers); *jobQueueDepth = virThreadPoolGetJobQueueDepth(srv->workers); - virObjectUnlock(srv); return 0; } @@ -1039,66 +1006,46 @@ virNetServerSetThreadPoolParameters(virNetServer *srv, long long int maxWorkers, long long int prioWorkers) { - int ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - virObjectLock(srv); - ret = virThreadPoolSetParameters(srv->workers, minWorkers, - maxWorkers, prioWorkers); - virObjectUnlock(srv); - - return ret; + return virThreadPoolSetParameters(srv->workers, minWorkers, + maxWorkers, prioWorkers); } size_t virNetServerGetMaxClients(virNetServer *srv) { - size_t ret; - - virObjectLock(srv); - ret = srv->nclients_max; - virObjectUnlock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - return ret; + return srv->nclients_max; } size_t virNetServerGetCurrentClients(virNetServer *srv) { - size_t ret; - - virObjectLock(srv); - ret = srv->nclients; - virObjectUnlock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - return ret; + return srv->nclients; } size_t virNetServerGetMaxUnauthClients(virNetServer *srv) { - size_t ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - virObjectLock(srv); - ret = srv->nclients_unauth_max; - virObjectUnlock(srv); - - return ret; + return srv->nclients_unauth_max; } size_t virNetServerGetCurrentUnauthClients(virNetServer *srv) { - size_t ret; - - virObjectLock(srv); - ret = srv->nclients_unauth; - virObjectUnlock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); - return ret; + return srv->nclients_unauth; } @@ -1106,17 +1053,15 @@ bool virNetServerNeedsAuth(virNetServer *srv, int auth) { - bool ret = false; + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); size_t i; - virObjectLock(srv); for (i = 0; i < srv->nservices; i++) { if (virNetServerServiceGetAuth(srv->services[i]) == auth) - ret = true; + return true; } - virObjectUnlock(srv); - return ret; + return false; } @@ -1127,8 +1072,7 @@ virNetServerGetClients(virNetServer *srv, size_t i; size_t nclients = 0; virNetServerClient **list = NULL; - - virObjectLock(srv); + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); for (i = 0; i < srv->nclients; i++) { virNetServerClient *client = virObjectRef(srv->clients[i]); @@ -1137,8 +1081,6 @@ virNetServerGetClients(virNetServer *srv, *clts = g_steal_pointer(&list); - virObjectUnlock(srv); - return nclients; } @@ -1147,23 +1089,17 @@ virNetServerClient * virNetServerGetClient(virNetServer *srv, unsigned long long id) { + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); size_t i; - virNetServerClient *ret = NULL; - - virObjectLock(srv); for (i = 0; i < srv->nclients; i++) { virNetServerClient *client = srv->clients[i]; if (virNetServerClientGetID(client) == id) - ret = virObjectRef(client); + return virObjectRef(client); } - virObjectUnlock(srv); - - if (!ret) - virReportError(VIR_ERR_NO_CLIENT, - _("No client with matching ID '%llu'"), id); - return ret; + virReportError(VIR_ERR_NO_CLIENT, _("No client with matching ID '%llu'"), id); + return NULL; } @@ -1172,13 +1108,9 @@ virNetServerSetClientLimits(virNetServer *srv, long long int maxClients, long long int maxClientsUnauth) { - int ret = -1; - size_t max, max_unauth; - - virObjectLock(srv); - - max = maxClients >= 0 ? maxClients : srv->nclients_max; - max_unauth = maxClientsUnauth >= 0 ? + VIR_LOCK_GUARD lock = virObjectLockGuard(srv); + size_t max = maxClients >= 0 ? maxClients : srv->nclients_max; + size_t max_unauth = maxClientsUnauth >= 0 ? maxClientsUnauth : srv->nclients_unauth_max; if (max < max_unauth) { @@ -1186,7 +1118,7 @@ virNetServerSetClientLimits(virNetServer *srv, _("The overall maximum number of clients must be " "greater than the maximum number of clients waiting " "for authentication")); - goto cleanup; + return -1; } if (maxClients >= 0) @@ -1197,10 +1129,7 @@ virNetServerSetClientLimits(virNetServer *srv, virNetServerCheckLimits(srv); - ret = 0; - cleanup: - virObjectUnlock(srv); - return ret; + return 0; } @@ -1226,30 +1155,24 @@ virNetServerGetTLSContext(virNetServer *srv) int virNetServerUpdateTlsFiles(virNetServer *srv) { - int ret = -1; - virNetTLSContext *ctxt = NULL; bool privileged = geteuid() == 0; + virNetTLSContext *ctxt = virNetServerGetTLSContext(srv); - ctxt = virNetServerGetTLSContext(srv); if (!ctxt) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no tls service found, unable to update tls files")); return -1; } - virObjectLock(srv); - virObjectLock(ctxt); - - if (virNetTLSContextReloadForServer(ctxt, !privileged)) { - VIR_DEBUG("failed to reload server's tls context"); - goto cleanup; + VIR_WITH_OBJECT_LOCK_GUARD(srv) { + VIR_WITH_OBJECT_LOCK_GUARD(ctxt) { + if (virNetTLSContextReloadForServer(ctxt, !privileged)) { + VIR_DEBUG("failed to reload server's tls context"); + return -1; + } + } } VIR_DEBUG("update tls files success"); - ret = 0; - - cleanup: - virObjectUnlock(ctxt); - virObjectUnlock(srv); - return ret; + return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/rpc/virnetserverclient.c | 432 +++++++++++++++-------------------- 1 file changed, 186 insertions(+), 246 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 7d5c0965b8..da9956f2b4 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -234,14 +234,12 @@ int virNetServerClientAddFilter(virNetServerClient *client, virNetServerClientFilterFunc func, void *opaque) { + VIR_LOCK_GUARD lock = virObjectLockGuard(client); virNetServerClientFilter *filter; virNetServerClientFilter **place; - int ret; filter = g_new0(virNetServerClientFilter, 1); - virObjectLock(client); - filter->id = client->nextFilterID++; filter->func = func; filter->opaque = opaque; @@ -251,21 +249,16 @@ int virNetServerClientAddFilter(virNetServerClient *client, place = &(*place)->next; *place = filter; - ret = filter->id; - - virObjectUnlock(client); - - return ret; + return filter->id; } void virNetServerClientRemoveFilter(virNetServerClient *client, int filterID) { + VIR_LOCK_GUARD lock = virObjectLockGuard(client); virNetServerClientFilter *tmp; virNetServerClientFilter *prev; - virObjectLock(client); - prev = NULL; tmp = client->filters; while (tmp) { @@ -281,8 +274,6 @@ void virNetServerClientRemoveFilter(virNetServerClient *client, prev = tmp; tmp = tmp->next; } - - virObjectUnlock(client); } @@ -322,19 +313,19 @@ virNetServerClientCheckAccess(virNetServerClient *client) static void virNetServerClientDispatchMessage(virNetServerClient *client, virNetMessage *msg) { - virObjectLock(client); - if (!client->dispatchFunc) { - virNetMessageFree(msg); - client->wantClose = true; - virObjectUnlock(client); - } else { - virObjectUnlock(client); - /* Accessing 'client' is safe, because virNetServerClientSetDispatcher - * only permits setting 'dispatchFunc' once, so if non-NULL, it will - * never change again - */ - client->dispatchFunc(client, msg, client->dispatchOpaque); + VIR_WITH_OBJECT_LOCK_GUARD(client) { + if (!client->dispatchFunc) { + virNetMessageFree(msg); + client->wantClose = true; + return; + } } + + /* Accessing 'client' is safe, because virNetServerClientSetDispatcher + * only permits setting 'dispatchFunc' once, so if non-NULL, it will + * never change again + */ + client->dispatchFunc(client, msg, client->dispatchOpaque); } @@ -343,13 +334,14 @@ static void virNetServerClientSockTimerFunc(int timer, { virNetServerClient *client = opaque; virNetMessage *msg = NULL; - virObjectLock(client); - virEventUpdateTimeout(timer, -1); - /* Although client->rx != NULL when this timer is enabled, it might have - * changed since the client was unlocked in the meantime. */ - if (client->rx) - msg = virNetServerClientDispatchRead(client); - virObjectUnlock(client); + + VIR_WITH_OBJECT_LOCK_GUARD(client) { + virEventUpdateTimeout(timer, -1); + /* Although client->rx != NULL when this timer is enabled, it might have + * changed since the client was unlocked in the meantime. */ + if (client->rx) + msg = virNetServerClientDispatchRead(client); + } if (msg) virNetServerClientDispatchMessage(client, msg); @@ -587,53 +579,45 @@ virJSONValue *virNetServerClientPreExecRestart(virNetServerClient *client) g_autoptr(virJSONValue) object = virJSONValueNewObject(); g_autoptr(virJSONValue) sock = NULL; g_autoptr(virJSONValue) priv = NULL; - - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); if (virJSONValueObjectAppendNumberUlong(object, "id", client->id) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendBoolean(object, "auth_pending", client->auth_pending) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendBoolean(object, "readonly", client->readonly) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendNumberUint(object, "nrequests_max", client->nrequests_max) < 0) - goto error; + return NULL; if (client->conn_time && virJSONValueObjectAppendNumberLong(object, "conn_time", client->conn_time) < 0) - goto error; + return NULL; if (!(sock = virNetSocketPreExecRestart(client->sock))) - goto error; + return NULL; if (virJSONValueObjectAppend(object, "sock", &sock) < 0) - goto error; + return NULL; if (!(priv = client->privateDataPreExecRestart(client, client->privateData))) - goto error; + return NULL; if (virJSONValueObjectAppend(object, "privateData", &priv) < 0) - goto error; + return NULL; - virObjectUnlock(client); return g_steal_pointer(&object); - - error: - virObjectUnlock(client); - return NULL; } int virNetServerClientGetAuth(virNetServerClient *client) { - int auth; - virObjectLock(client); - auth = client->auth; - virObjectUnlock(client); - return auth; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return client->auth; } @@ -647,11 +631,9 @@ virNetServerClientSetAuthLocked(virNetServerClient *client, bool virNetServerClientGetReadonly(virNetServerClient *client) { - bool readonly; - virObjectLock(client); - readonly = client->readonly; - virObjectUnlock(client); - return readonly; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return client->readonly; } @@ -659,9 +641,9 @@ void virNetServerClientSetReadonly(virNetServerClient *client, bool readonly) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + client->readonly = readonly; - virObjectUnlock(client); } @@ -677,52 +659,48 @@ long long virNetServerClientGetTimestamp(virNetServerClient *client) bool virNetServerClientHasTLSSession(virNetServerClient *client) { - bool has; - virObjectLock(client); - has = client->tls ? true : false; - virObjectUnlock(client); - return has; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return !!client->tls; } virNetTLSSession *virNetServerClientGetTLSSession(virNetServerClient *client) { - virNetTLSSession *tls; - virObjectLock(client); - tls = client->tls; - virObjectUnlock(client); - return tls; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return client->tls; } int virNetServerClientGetTLSKeySize(virNetServerClient *client) { - int size = 0; - virObjectLock(client); - if (client->tls) - size = virNetTLSSessionGetKeySize(client->tls); - virObjectUnlock(client); - return size; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + if (!client->tls) + return 0; + + return virNetTLSSessionGetKeySize(client->tls); } int virNetServerClientGetFD(virNetServerClient *client) { - int fd = -1; - virObjectLock(client); - if (client->sock) - fd = virNetSocketGetFD(client->sock); - virObjectUnlock(client); - return fd; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + if (!client->sock) + return -1; + + return virNetSocketGetFD(client->sock); } bool virNetServerClientIsLocal(virNetServerClient *client) { - bool local = false; - virObjectLock(client); - if (client->sock) - local = virNetSocketIsLocal(client->sock); - virObjectUnlock(client); - return local; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + if (!client->sock) + return false; + + return virNetSocketIsLocal(client->sock); } @@ -730,14 +708,12 @@ int virNetServerClientGetUNIXIdentity(virNetServerClient *client, uid_t *uid, gid_t *gid, pid_t *pid, unsigned long long *timestamp) { - int ret = -1; - virObjectLock(client); - if (client->sock) - ret = virNetSocketGetUNIXIdentity(client->sock, - uid, gid, pid, - timestamp); - virObjectUnlock(client); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + if (!client->sock) + return -1; + + return virNetSocketGetUNIXIdentity(client->sock, uid, gid, pid, timestamp); } @@ -806,56 +782,60 @@ virNetServerClientCreateIdentity(virNetServerClient *client) virIdentity *virNetServerClientGetIdentity(virNetServerClient *client) { - virIdentity *ret = NULL; - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + if (!client->identity) client->identity = virNetServerClientCreateIdentity(client); - if (client->identity) - ret = g_object_ref(client->identity); - virObjectUnlock(client); - return ret; + + if (!client->identity) + return NULL; + + return g_object_ref(client->identity); } void virNetServerClientSetIdentity(virNetServerClient *client, virIdentity *identity) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + g_clear_object(&client->identity); client->identity = identity; if (client->identity) g_object_ref(client->identity); - virObjectUnlock(client); } int virNetServerClientGetSELinuxContext(virNetServerClient *client, char **context) { - int ret = 0; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + *context = NULL; - virObjectLock(client); - if (client->sock) - ret = virNetSocketGetSELinuxContext(client->sock, context); - virObjectUnlock(client); - return ret; + + if (!client->sock) + return 0; + + return virNetSocketGetSELinuxContext(client->sock, context); } bool virNetServerClientIsSecure(virNetServerClient *client) { - bool secure = false; - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + if (client->tls) - secure = true; + return true; + #if WITH_SASL if (client->sasl) - secure = true; + return true; #endif + if (client->sock && virNetSocketIsLocal(client->sock)) - secure = true; - virObjectUnlock(client); - return secure; + return true; + + return false; } @@ -863,53 +843,47 @@ bool virNetServerClientIsSecure(virNetServerClient *client) void virNetServerClientSetSASLSession(virNetServerClient *client, virNetSASLSession *sasl) { + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + /* We don't set the sasl session on the socket here * because we need to send out the auth confirmation * in the clear. Only once we complete the next 'tx' * operation do we switch to SASL mode */ - virObjectLock(client); client->sasl = virObjectRef(sasl); - virObjectUnlock(client); } virNetSASLSession *virNetServerClientGetSASLSession(virNetServerClient *client) { - virNetSASLSession *sasl; - virObjectLock(client); - sasl = client->sasl; - virObjectUnlock(client); - return sasl; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return client->sasl; } bool virNetServerClientHasSASLSession(virNetServerClient *client) { - bool has = false; - virObjectLock(client); - has = !!client->sasl; - virObjectUnlock(client); - return has; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return !!client->sasl; } #endif void *virNetServerClientGetPrivateData(virNetServerClient *client) { - void *data; - virObjectLock(client); - data = client->privateData; - virObjectUnlock(client); - return data; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return client->privateData; } void virNetServerClientSetCloseHook(virNetServerClient *client, virNetServerClientCloseFunc cf) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + client->privateDataCloseFunc = cf; - virObjectUnlock(client); } @@ -917,7 +891,8 @@ void virNetServerClientSetDispatcher(virNetServerClient *client, virNetServerClientDispatchFunc func, void *opaque) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + /* Only set dispatcher if not already set, to avoid race * with dispatch code that runs without locks held */ @@ -925,7 +900,6 @@ void virNetServerClientSetDispatcher(virNetServerClient *client, client->dispatchFunc = func; client->dispatchOpaque = opaque; } - virObjectUnlock(client); } @@ -1042,9 +1016,9 @@ virNetServerClientCloseLocked(virNetServerClient *client) void virNetServerClientClose(virNetServerClient *client) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + virNetServerClientCloseLocked(client); - virObjectUnlock(client); } @@ -1057,16 +1031,16 @@ virNetServerClientIsClosedLocked(virNetServerClient *client) void virNetServerClientDelayedClose(virNetServerClient *client) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + client->delayedClose = true; - virObjectUnlock(client); } void virNetServerClientImmediateClose(virNetServerClient *client) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + client->wantClose = true; - virObjectUnlock(client); } @@ -1079,49 +1053,46 @@ virNetServerClientWantCloseLocked(virNetServerClient *client) int virNetServerClientInit(virNetServerClient *client) { - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + int ret = -1; if (!client->tlsCtxt) { /* Plain socket, so prepare to read first message */ if (virNetServerClientRegisterEvent(client) < 0) goto error; - } else { - int ret; + return 0; + } - if (!(client->tls = virNetTLSSessionNew(client->tlsCtxt, - NULL))) - goto error; + if (!(client->tls = virNetTLSSessionNew(client->tlsCtxt, NULL))) + goto error; - virNetSocketSetTLSSession(client->sock, - client->tls); + virNetSocketSetTLSSession(client->sock, client->tls); - /* Begin the TLS handshake. */ - virObjectLock(client->tlsCtxt); + /* Begin the TLS handshake. */ + VIR_WITH_OBJECT_LOCK_GUARD(client->tlsCtxt) { ret = virNetTLSSessionHandshake(client->tls); - virObjectUnlock(client->tlsCtxt); - if (ret == 0) { - /* Unlikely, but ... Next step is to check the certificate. */ - if (virNetServerClientCheckAccess(client) < 0) - goto error; - - /* Handshake & cert check OK, so prepare to read first message */ - if (virNetServerClientRegisterEvent(client) < 0) - goto error; - } else if (ret > 0) { - /* Most likely, need to do more handshake data */ - if (virNetServerClientRegisterEvent(client) < 0) - goto error; - } else { + } + + if (ret == 0) { + /* Unlikely, but ... Next step is to check the certificate. */ + if (virNetServerClientCheckAccess(client) < 0) goto error; - } + + /* Handshake & cert check OK, so prepare to read first message */ + if (virNetServerClientRegisterEvent(client) < 0) + goto error; + } else if (ret > 0) { + /* Most likely, need to do more handshake data */ + if (virNetServerClientRegisterEvent(client) < 0) + goto error; + } else { + goto error; } - virObjectUnlock(client); return 0; error: client->wantClose = true; - virObjectUnlock(client); return -1; } @@ -1406,11 +1377,13 @@ virNetServerClientDispatchWrite(virNetServerClient *client) static void virNetServerClientDispatchHandshake(virNetServerClient *client) { - int ret; + int ret = -1; + /* Continue the handshake. */ - virObjectLock(client->tlsCtxt); - ret = virNetTLSSessionHandshake(client->tls); - virObjectUnlock(client->tlsCtxt); + VIR_WITH_OBJECT_LOCK_GUARD(client->tlsCtxt) { + ret = virNetTLSSessionHandshake(client->tls); + } + if (ret == 0) { /* Finished. Next step is to check the certificate. */ if (virNetServerClientCheckAccess(client) < 0) @@ -1435,36 +1408,29 @@ virNetServerClientDispatchEvent(virNetSocket *sock, int events, void *opaque) virNetServerClient *client = opaque; virNetMessage *msg = NULL; - virObjectLock(client); - - if (client->sock != sock) { - virNetSocketRemoveIOCallback(sock); - virObjectUnlock(client); - return; - } - - if (events & (VIR_EVENT_HANDLE_WRITABLE | - VIR_EVENT_HANDLE_READABLE)) { - if (client->tls && - virNetTLSSessionGetHandshakeStatus(client->tls) != - VIR_NET_TLS_HANDSHAKE_COMPLETE) { - virNetServerClientDispatchHandshake(client); - } else { - if (events & VIR_EVENT_HANDLE_WRITABLE) - virNetServerClientDispatchWrite(client); - if (events & VIR_EVENT_HANDLE_READABLE && - client->rx) - msg = virNetServerClientDispatchRead(client); + VIR_WITH_OBJECT_LOCK_GUARD(client) { + if (client->sock != sock) { + virNetSocketRemoveIOCallback(sock); + return; } - } - /* NB, will get HANGUP + READABLE at same time upon - * disconnect */ - if (events & (VIR_EVENT_HANDLE_ERROR | - VIR_EVENT_HANDLE_HANGUP)) - client->wantClose = true; + if (events & (VIR_EVENT_HANDLE_WRITABLE | VIR_EVENT_HANDLE_READABLE)) { + if (client->tls && + virNetTLSSessionGetHandshakeStatus(client->tls) != + VIR_NET_TLS_HANDSHAKE_COMPLETE) { + virNetServerClientDispatchHandshake(client); + } else { + if (events & VIR_EVENT_HANDLE_WRITABLE) + virNetServerClientDispatchWrite(client); + if ((events & VIR_EVENT_HANDLE_READABLE) && client->rx) + msg = virNetServerClientDispatchRead(client); + } + } - virObjectUnlock(client); + /* NB, will get HANGUP + READABLE at same time upon disconnect */ + if (events & (VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP)) + client->wantClose = true; + } if (msg) virNetServerClientDispatchMessage(client, msg); @@ -1499,24 +1465,18 @@ virNetServerClientSendMessageLocked(virNetServerClient *client, int virNetServerClientSendMessage(virNetServerClient *client, virNetMessage *msg) { - int ret; - - virObjectLock(client); - ret = virNetServerClientSendMessageLocked(client, msg); - virObjectUnlock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); - return ret; + return virNetServerClientSendMessageLocked(client, msg); } bool virNetServerClientIsAuthenticated(virNetServerClient *client) { - bool authenticated; - virObjectLock(client); - authenticated = virNetServerClientAuthMethodImpliesAuthenticated(client->auth); - virObjectUnlock(client); - return authenticated; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); + + return virNetServerClientAuthMethodImpliesAuthenticated(client->auth); } @@ -1556,57 +1516,44 @@ virNetServerClientInitKeepAlive(virNetServerClient *client, int interval, unsigned int count) { + VIR_LOCK_GUARD lock = virObjectLockGuard(client); virKeepAlive *ka; - int ret = -1; - - virObjectLock(client); if (!(ka = virKeepAliveNew(interval, count, client, virNetServerClientKeepAliveSendCB, virNetServerClientKeepAliveDeadCB, virObjectFreeCallback))) - goto cleanup; + return -1; + /* keepalive object has a reference to client */ virObjectRef(client); client->keepalive = ka; - ret = 0; - cleanup: - virObjectUnlock(client); - - return ret; + return 0; } int virNetServerClientStartKeepAlive(virNetServerClient *client) { - int ret = -1; - - virObjectLock(client); + VIR_LOCK_GUARD lock = virObjectLockGuard(client); /* The connection might have been closed before we got here and thus the * keepalive object could have been removed too. */ if (!client->keepalive) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("connection not open")); - goto cleanup; + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + return -1; } - ret = virKeepAliveStart(client->keepalive, 0, 0); - - cleanup: - virObjectUnlock(client); - return ret; + return virKeepAliveStart(client->keepalive, 0, 0); } int virNetServerClientGetTransport(virNetServerClient *client) { + VIR_LOCK_GUARD lock = virObjectLockGuard(client); int ret = -1; - virObjectLock(client); - if (client->sock && virNetSocketIsLocal(client->sock)) ret = VIR_CLIENT_TRANS_UNIX; else @@ -1615,8 +1562,6 @@ virNetServerClientGetTransport(virNetServerClient *client) if (client->tls) ret = VIR_CLIENT_TRANS_TLS; - virObjectUnlock(client); - return ret; } @@ -1625,16 +1570,15 @@ virNetServerClientGetInfo(virNetServerClient *client, bool *readonly, char **sock_addr, virIdentity **identity) { - int ret = -1; + VIR_LOCK_GUARD lock = virObjectLockGuard(client); const char *addr; - virObjectLock(client); *readonly = client->readonly; if (!(addr = virNetServerClientRemoteAddrStringURI(client))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No network socket associated with client")); - goto cleanup; + return -1; } *sock_addr = g_strdup(addr); @@ -1642,15 +1586,11 @@ virNetServerClientGetInfo(virNetServerClient *client, if (!client->identity) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No identity information available for client")); - goto cleanup; + return -1; } *identity = g_object_ref(client->identity); - - ret = 0; - cleanup: - virObjectUnlock(client); - return ret; + return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/security/security_manager.c | 524 +++++++++++++------------------- 1 file changed, 215 insertions(+), 309 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f9c942de08..572e400a48 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -254,13 +254,12 @@ virSecurityManagerPostFork(virSecurityManager *mgr) int virSecurityManagerTransactionStart(virSecurityManager *mgr) { - int ret = 0; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - virObjectLock(mgr); - if (mgr->drv->transactionStart) - ret = mgr->drv->transactionStart(mgr); - virObjectUnlock(mgr); - return ret; + if (!mgr->drv->transactionStart) + return 0; + + return mgr->drv->transactionStart(mgr); } @@ -291,13 +290,12 @@ virSecurityManagerTransactionCommit(virSecurityManager *mgr, pid_t pid, bool lock) { - int ret = 0; + VIR_LOCK_GUARD lockguard = virObjectLockGuard(mgr); - virObjectLock(mgr); - if (mgr->drv->transactionCommit) - ret = mgr->drv->transactionCommit(mgr, pid, lock); - virObjectUnlock(mgr); - return ret; + if (!mgr->drv->transactionCommit) + return 0; + + return mgr->drv->transactionCommit(mgr, pid, lock); } @@ -310,10 +308,10 @@ virSecurityManagerTransactionCommit(virSecurityManager *mgr, void virSecurityManagerTransactionAbort(virSecurityManager *mgr) { - virObjectLock(mgr); + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + if (mgr->drv->transactionAbort) mgr->drv->transactionAbort(mgr); - virObjectUnlock(mgr); } @@ -341,32 +339,28 @@ virSecurityManagerGetDriver(virSecurityManager *mgr) const char * virSecurityManagerGetDOI(virSecurityManager *mgr) { - if (mgr->drv->getDOI) { - const char *ret; - virObjectLock(mgr); - ret = mgr->drv->getDOI(mgr); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->getDOI) { + virReportUnsupportedError(); + return NULL; } - virReportUnsupportedError(); - return NULL; + return mgr->drv->getDOI(mgr); } const char * virSecurityManagerGetModel(virSecurityManager *mgr) { - if (mgr->drv->getModel) { - const char *ret; - virObjectLock(mgr); - ret = mgr->drv->getModel(mgr); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->getModel) { + virReportUnsupportedError(); + return NULL; } - virReportUnsupportedError(); - return NULL; + return mgr->drv->getModel(mgr); } @@ -375,15 +369,12 @@ const char * virSecurityManagerGetBaseLabel(virSecurityManager *mgr, int virtType) { - if (mgr->drv->getBaseLabel) { - const char *ret; - virObjectLock(mgr); - ret = mgr->drv->getBaseLabel(mgr, virtType); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return NULL; + if (!mgr->drv->getBaseLabel) + return NULL; + + return mgr->drv->getBaseLabel(mgr, virtType); } @@ -425,16 +416,14 @@ virSecurityManagerRestoreImageLabel(virSecurityManager *mgr, virStorageSource *src, virSecurityDomainImageLabelFlags flags) { - if (mgr->drv->domainRestoreSecurityImageLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src, flags); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainRestoreSecurityImageLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src, flags); } @@ -464,15 +453,12 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, virStorageSource *src, virStorageSource *dst) { - if (mgr->drv->domainMoveImageMetadata) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainMoveImageMetadata(mgr, pid, src, dst); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return 0; + if (!mgr->drv->domainMoveImageMetadata) + return 0; + + return mgr->drv->domainMoveImageMetadata(mgr, pid, src, dst); } @@ -480,16 +466,14 @@ int virSecurityManagerSetDaemonSocketLabel(virSecurityManager *mgr, virDomainDef *vm) { - if (mgr->drv->domainSetSecurityDaemonSocketLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityDaemonSocketLabel(mgr, vm); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityDaemonSocketLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityDaemonSocketLabel(mgr, vm); } @@ -497,16 +481,14 @@ int virSecurityManagerSetSocketLabel(virSecurityManager *mgr, virDomainDef *vm) { - if (mgr->drv->domainSetSecuritySocketLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecuritySocketLabel(mgr, vm); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecuritySocketLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecuritySocketLabel(mgr, vm); } @@ -514,16 +496,14 @@ int virSecurityManagerClearSocketLabel(virSecurityManager *mgr, virDomainDef *vm) { - if (mgr->drv->domainClearSecuritySocketLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainClearSecuritySocketLabel(mgr, vm); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainClearSecuritySocketLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainClearSecuritySocketLabel(mgr, vm); } @@ -544,16 +524,14 @@ virSecurityManagerSetImageLabel(virSecurityManager *mgr, virStorageSource *src, virSecurityDomainImageLabelFlags flags) { - if (mgr->drv->domainSetSecurityImageLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src, flags); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityImageLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityImageLabel(mgr, vm, src, flags); } @@ -563,16 +541,14 @@ virSecurityManagerRestoreHostdevLabel(virSecurityManager *mgr, virDomainHostdevDef *dev, const char *vroot) { - if (mgr->drv->domainRestoreSecurityHostdevLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityHostdevLabel(mgr, vm, dev, vroot); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainRestoreSecurityHostdevLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainRestoreSecurityHostdevLabel(mgr, vm, dev, vroot); } @@ -582,16 +558,14 @@ virSecurityManagerSetHostdevLabel(virSecurityManager *mgr, virDomainHostdevDef *dev, const char *vroot) { - if (mgr->drv->domainSetSecurityHostdevLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityHostdevLabel(mgr, vm, dev, vroot); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityHostdevLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityHostdevLabel(mgr, vm, dev, vroot); } @@ -600,15 +574,12 @@ virSecurityManagerSetSavedStateLabel(virSecurityManager *mgr, virDomainDef *vm, const char *savefile) { - if (mgr->drv->domainSetSavedStateLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSavedStateLabel(mgr, vm, savefile); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return 0; + if (!mgr->drv->domainSetSavedStateLabel) + return 0; + + return mgr->drv->domainSetSavedStateLabel(mgr, vm, savefile); } @@ -617,15 +588,12 @@ virSecurityManagerRestoreSavedStateLabel(virSecurityManager *mgr, virDomainDef *vm, const char *savefile) { - if (mgr->drv->domainRestoreSavedStateLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSavedStateLabel(mgr, vm, savefile); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return 0; + if (!mgr->drv->domainRestoreSavedStateLabel) + return 0; + + return mgr->drv->domainRestoreSavedStateLabel(mgr, vm, savefile); } @@ -713,16 +681,14 @@ virSecurityManagerReserveLabel(virSecurityManager *mgr, virDomainDef *vm, pid_t pid) { - if (mgr->drv->domainReserveSecurityLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainReserveSecurityLabel(mgr, vm, pid); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainReserveSecurityLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainReserveSecurityLabel(mgr, vm, pid); } @@ -730,16 +696,14 @@ int virSecurityManagerReleaseLabel(virSecurityManager *mgr, virDomainDef *vm) { - if (mgr->drv->domainReleaseSecurityLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainReleaseSecurityLabel(mgr, vm); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainReleaseSecurityLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainReleaseSecurityLabel(mgr, vm); } @@ -857,18 +821,15 @@ virSecurityManagerSetAllLabel(virSecurityManager *mgr, bool chardevStdioLogd, bool migrated) { - if (mgr->drv->domainSetSecurityAllLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, incomingPath, - chardevStdioLogd, - migrated); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityAllLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityAllLabel(mgr, vm, incomingPath, + chardevStdioLogd, migrated); } @@ -878,17 +839,15 @@ virSecurityManagerRestoreAllLabel(virSecurityManager *mgr, bool migrated, bool chardevStdioLogd) { - if (mgr->drv->domainRestoreSecurityAllLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated, - chardevStdioLogd); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainRestoreSecurityAllLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated, + chardevStdioLogd); } int @@ -897,16 +856,14 @@ virSecurityManagerGetProcessLabel(virSecurityManager *mgr, pid_t pid, virSecurityLabelPtr sec) { - if (mgr->drv->domainGetSecurityProcessLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainGetSecurityProcessLabel(mgr, vm, pid, sec); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainGetSecurityProcessLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainGetSecurityProcessLabel(mgr, vm, pid, sec); } @@ -914,16 +871,14 @@ int virSecurityManagerSetProcessLabel(virSecurityManager *mgr, virDomainDef *vm) { - if (mgr->drv->domainSetSecurityProcessLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityProcessLabel(mgr, vm); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityProcessLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityProcessLabel(mgr, vm); } @@ -957,12 +912,10 @@ virSecurityManagerVerify(virSecurityManager *mgr, if (secdef == NULL || secdef->model == NULL) return 0; - if (mgr->drv->domainSecurityVerify) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSecurityVerify(mgr, def); - virObjectUnlock(mgr); - return ret; + VIR_WITH_OBJECT_LOCK_GUARD(mgr) { + if (mgr->drv->domainSecurityVerify) { + return mgr->drv->domainSecurityVerify(mgr, def); + } } virReportUnsupportedError(); @@ -975,16 +928,14 @@ virSecurityManagerSetImageFDLabel(virSecurityManager *mgr, virDomainDef *vm, int fd) { - if (mgr->drv->domainSetSecurityImageFDLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityImageFDLabel(mgr, vm, fd); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityImageFDLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityImageFDLabel(mgr, vm, fd); } @@ -993,16 +944,14 @@ virSecurityManagerSetTapFDLabel(virSecurityManager *mgr, virDomainDef *vm, int fd) { - if (mgr->drv->domainSetSecurityTapFDLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityTapFDLabel(mgr, vm, fd); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityTapFDLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityTapFDLabel(mgr, vm, fd); } @@ -1010,16 +959,14 @@ char * virSecurityManagerGetMountOptions(virSecurityManager *mgr, virDomainDef *vm) { - if (mgr->drv->domainGetSecurityMountOptions) { - char *ret; - virObjectLock(mgr); - ret = mgr->drv->domainGetSecurityMountOptions(mgr, vm); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainGetSecurityMountOptions) { + virReportUnsupportedError(); + return NULL; } - virReportUnsupportedError(); - return NULL; + return mgr->drv->domainGetSecurityMountOptions(mgr, vm); } @@ -1059,15 +1006,12 @@ virSecurityManagerDomainSetPathLabel(virSecurityManager *mgr, const char *path, bool allowSubtree) { - if (mgr->drv->domainSetPathLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetPathLabel(mgr, vm, path, allowSubtree); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return 0; + if (!mgr->drv->domainSetPathLabel) + return 0; + + return mgr->drv->domainSetPathLabel(mgr, vm, path, allowSubtree); } @@ -1088,15 +1032,12 @@ virSecurityManagerDomainSetPathLabelRO(virSecurityManager *mgr, virDomainDef *vm, const char *path) { - if (mgr->drv->domainSetPathLabelRO) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetPathLabelRO(mgr, vm, path); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return 0; + if (!mgr->drv->domainSetPathLabelRO) + return 0; + + return mgr->drv->domainSetPathLabelRO(mgr, vm, path); } /** @@ -1115,15 +1056,12 @@ virSecurityManagerDomainRestorePathLabel(virSecurityManager *mgr, virDomainDef *vm, const char *path) { - if (mgr->drv->domainRestorePathLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestorePathLabel(mgr, vm, path); - virObjectUnlock(mgr); - return ret; - } + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return 0; + if (!mgr->drv->domainRestorePathLabel) + return 0; + + return mgr->drv->domainRestorePathLabel(mgr, vm, path); } @@ -1143,16 +1081,14 @@ virSecurityManagerSetMemoryLabel(virSecurityManager *mgr, virDomainDef *vm, virDomainMemoryDef *mem) { - if (mgr->drv->domainSetSecurityMemoryLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityMemoryLabel(mgr, vm, mem); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityMemoryLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityMemoryLabel(mgr, vm, mem); } @@ -1171,16 +1107,14 @@ virSecurityManagerRestoreMemoryLabel(virSecurityManager *mgr, virDomainDef *vm, virDomainMemoryDef *mem) { - if (mgr->drv->domainRestoreSecurityMemoryLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityMemoryLabel(mgr, vm, mem); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainRestoreSecurityMemoryLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainRestoreSecurityMemoryLabel(mgr, vm, mem); } @@ -1189,16 +1123,14 @@ virSecurityManagerSetInputLabel(virSecurityManager *mgr, virDomainDef *vm, virDomainInputDef *input) { - if (mgr->drv->domainSetSecurityInputLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityInputLabel(mgr, vm, input); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityInputLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityInputLabel(mgr, vm, input); } @@ -1207,16 +1139,14 @@ virSecurityManagerRestoreInputLabel(virSecurityManager *mgr, virDomainDef *vm, virDomainInputDef *input) { - if (mgr->drv->domainRestoreSecurityInputLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityInputLabel(mgr, vm, input); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainRestoreSecurityInputLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainRestoreSecurityInputLabel(mgr, vm, input); } @@ -1226,17 +1156,15 @@ virSecurityManagerSetChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd) { - if (mgr->drv->domainSetSecurityChardevLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityChardevLabel(mgr, def, dev_source, - chardevStdioLogd); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainSetSecurityChardevLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainSetSecurityChardevLabel(mgr, def, dev_source, + chardevStdioLogd); } @@ -1246,17 +1174,15 @@ virSecurityManagerRestoreChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd) { - if (mgr->drv->domainRestoreSecurityChardevLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityChardevLabel(mgr, def, dev_source, - chardevStdioLogd); - virObjectUnlock(mgr); - return ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainRestoreSecurityChardevLabel) { + virReportUnsupportedError(); + return -1; } - virReportUnsupportedError(); - return -1; + return mgr->drv->domainRestoreSecurityChardevLabel(mgr, def, dev_source, + chardevStdioLogd); } @@ -1264,17 +1190,12 @@ int virSecurityManagerSetTPMLabels(virSecurityManager *mgr, virDomainDef *vm) { - int ret; - - if (mgr->drv->domainSetSecurityTPMLabels) { - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityTPMLabels(mgr, vm); - virObjectUnlock(mgr); + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return ret; - } + if (!mgr->drv->domainSetSecurityTPMLabels) + return 0; - return 0; + return mgr->drv->domainSetSecurityTPMLabels(mgr, vm); } @@ -1282,17 +1203,12 @@ int virSecurityManagerRestoreTPMLabels(virSecurityManager *mgr, virDomainDef *vm) { - int ret; + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - if (mgr->drv->domainRestoreSecurityTPMLabels) { - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityTPMLabels(mgr, vm); - virObjectUnlock(mgr); - - return ret; - } + if (!mgr->drv->domainRestoreSecurityTPMLabels) + return 0; - return 0; + return mgr->drv->domainRestoreSecurityTPMLabels(mgr, vm); } @@ -1301,17 +1217,12 @@ virSecurityManagerSetNetdevLabel(virSecurityManager *mgr, virDomainDef *vm, virDomainNetDef *net) { - int ret; - - if (mgr->drv->domainSetSecurityNetdevLabel) { - virObjectLock(mgr); - ret = mgr->drv->domainSetSecurityNetdevLabel(mgr, vm, net); - virObjectUnlock(mgr); + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return ret; - } + if (!mgr->drv->domainSetSecurityNetdevLabel) + return 0; - return 0; + return mgr->drv->domainSetSecurityNetdevLabel(mgr, vm, net); } @@ -1320,17 +1231,12 @@ virSecurityManagerRestoreNetdevLabel(virSecurityManager *mgr, virDomainDef *vm, virDomainNetDef *net) { - int ret; - - if (mgr->drv->domainRestoreSecurityNetdevLabel) { - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityNetdevLabel(mgr, vm, net); - virObjectUnlock(mgr); + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); - return ret; - } + if (!mgr->drv->domainRestoreSecurityNetdevLabel) + return 0; - return 0; + return mgr->drv->domainRestoreSecurityNetdevLabel(mgr, vm, net); } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/vz/vz_driver.c | 50 +++++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2107785ab2..82323cb1b2 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2018,53 +2018,43 @@ vzConnectRegisterCloseCallback(virConnectPtr conn, virFreeCallback freecb) { struct _vzConn *privconn = conn->privateData; - int ret = -1; if (virConnectRegisterCloseCallbackEnsureACL(conn) < 0) return -1; - virObjectLock(privconn->driver); + VIR_WITH_OBJECT_LOCK_GUARD(privconn->driver) { + if (virConnectCloseCallbackDataGetCallback(privconn->closeCallback) != NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("A close callback is already registered")); + return -1; + } - if (virConnectCloseCallbackDataGetCallback(privconn->closeCallback) != NULL) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("A close callback is already registered")); - goto cleanup; + virConnectCloseCallbackDataRegister(privconn->closeCallback, conn, cb, + opaque, freecb); } - virConnectCloseCallbackDataRegister(privconn->closeCallback, conn, cb, - opaque, freecb); - ret = 0; - - cleanup: - virObjectUnlock(privconn->driver); - - return ret; + return 0; } static int vzConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb) { struct _vzConn *privconn = conn->privateData; - int ret = -1; if (virConnectUnregisterCloseCallbackEnsureACL(conn) < 0) return -1; - virObjectLock(privconn->driver); + VIR_WITH_OBJECT_LOCK_GUARD(privconn->driver) { + if (virConnectCloseCallbackDataGetCallback(privconn->closeCallback) != cb) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("A different callback was requested")); + return -1; + } - if (virConnectCloseCallbackDataGetCallback(privconn->closeCallback) != cb) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("A different callback was requested")); - goto cleanup; + virConnectCloseCallbackDataUnregister(privconn->closeCallback, cb); } - virConnectCloseCallbackDataUnregister(privconn->closeCallback, cb); - ret = 0; - - cleanup: - virObjectUnlock(privconn->driver); - - return ret; + return 0; } static int vzDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, @@ -3795,9 +3785,9 @@ vzConnectGetAllDomainStats(virConnectPtr conn, virDomainStatsRecordPtr tmp; virDomainObj *dom = doms[i]; - virObjectLock(dom); - tmp = vzDomainGetAllStats(conn, dom); - virObjectUnlock(dom); + VIR_WITH_OBJECT_LOCK_GUARD(dom) { + tmp = vzDomainGetAllStats(conn, dom); + } if (!tmp) goto cleanup; -- 2.31.1
On 4/12/22 16:57, Tim Wiederhake wrote:
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management.
Tim Wiederhake (10): virstorageobj: Replace deprecated virHash functions virStoragePoolObjAddVol: Simplify error path virStorageVolObjEndAPI: Remove virStorageVolObjNew: Move locking to usage site virstorageobject: Use automatic mutex management virNetServerProcessClients: Remove goto virnetserver: Use automatic mutex management virnetserverclient: Use automatic mutex management security_manager: Use automatic mutex management vz_driver: Use automatic mutex management
src/conf/virstorageobj.c | 185 +++++------ src/rpc/virnetserver.c | 295 +++++++----------- src/rpc/virnetserverclient.c | 432 ++++++++++++-------------- src/security/security_manager.c | 524 +++++++++++++------------------- src/vz/vz_driver.c | 50 ++- 5 files changed, 604 insertions(+), 882 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake