[libvirt] [PATCH 0/4] Change per-connection hashes to be indexed by UUIDs
While working at the area, I also made few other cleanups... Jiri Denemark (4): Index hashes by UUID instead of name Remove unnecessary check for non-NULL uuid Do not free static buffer with UUID Misc cleanups src/datatypes.c | 83 ++++++++++++++++++++++++++++-------------------------- 1 files changed, 43 insertions(+), 40 deletions(-)
Per-connection hashes for domains, networks, storage pools and network filter pools were indexed by names which was not the best choice. UUIDs are better identifiers, so lets use them. --- src/datatypes.c | 64 +++++++++++++++++++++++++++++++------------------------ 1 files changed, 36 insertions(+), 28 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 88ad695..20b2d74 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -343,6 +343,7 @@ virUnrefConnect(virConnectPtr conn) { virDomainPtr virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { virDomainPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -350,10 +351,9 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { } virMutexLock(&conn->lock); - /* TODO search by UUID first as they are better differentiators */ + virUUIDFormat(uuid, uuidstr); - ret = (virDomainPtr) virHashLookup(conn->domains, name); - /* TODO check the UUID */ + ret = (virDomainPtr) virHashLookup(conn->domains, uuidstr); if (ret == NULL) { if (VIR_ALLOC(ret) < 0) { virMutexUnlock(&conn->lock); @@ -373,7 +373,7 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); ret->snapshots = virHashCreate(20); - if (virHashAddEntry(conn->domains, name, ret) < 0) { + if (virHashAddEntry(conn->domains, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to add domain to connection hash table")); @@ -411,10 +411,12 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { static void virReleaseDomain(virDomainPtr domain) { virConnectPtr conn = domain->conn; - DEBUG("release domain %p %s", domain, domain->name); + char uuidstr[VIR_UUID_STRING_BUFLEN]; - /* TODO search by UUID first as they are better differentiators */ - if (virHashRemoveEntry(conn->domains, domain->name, NULL) < 0) { + virUUIDFormat(domain->uuid, uuidstr); + DEBUG("release domain %p %s %s", domain, domain->name, uuidstr); + + if (virHashRemoveEntry(conn->domains, uuidstr, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain missing from connection hash table")); @@ -488,6 +490,7 @@ virUnrefDomain(virDomainPtr domain) { virNetworkPtr virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { virNetworkPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -495,10 +498,9 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { } virMutexLock(&conn->lock); - /* TODO search by UUID first as they are better differentiators */ + virUUIDFormat(uuid, uuidstr); - ret = (virNetworkPtr) virHashLookup(conn->networks, name); - /* TODO check the UUID */ + ret = (virNetworkPtr) virHashLookup(conn->networks, uuidstr); if (ret == NULL) { if (VIR_ALLOC(ret) < 0) { virMutexUnlock(&conn->lock); @@ -516,7 +518,7 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { if (uuid != NULL) memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); - if (virHashAddEntry(conn->networks, name, ret) < 0) { + if (virHashAddEntry(conn->networks, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to add network to connection hash table")); @@ -551,10 +553,12 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { static void virReleaseNetwork(virNetworkPtr network) { virConnectPtr conn = network->conn; - DEBUG("release network %p %s", network, network->name); + char uuidstr[VIR_UUID_STRING_BUFLEN]; - /* TODO search by UUID first as they are better differentiators */ - if (virHashRemoveEntry(conn->networks, network->name, NULL) < 0) { + virUUIDFormat(network->uuid, uuidstr); + DEBUG("release network %p %s %s", network, network->name, uuidstr); + + if (virHashRemoveEntry(conn->networks, uuidstr, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("network missing from connection hash table")); @@ -808,6 +812,7 @@ virUnrefInterface(virInterfacePtr iface) { virStoragePoolPtr virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uuid) { virStoragePoolPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -815,10 +820,9 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui } virMutexLock(&conn->lock); - /* TODO search by UUID first as they are better differentiators */ + virUUIDFormat(uuid, uuidstr); - ret = (virStoragePoolPtr) virHashLookup(conn->storagePools, name); - /* TODO check the UUID */ + ret = (virStoragePoolPtr) virHashLookup(conn->storagePools, uuidstr); if (ret == NULL) { if (VIR_ALLOC(ret) < 0) { virMutexUnlock(&conn->lock); @@ -836,7 +840,7 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui if (uuid != NULL) memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); - if (virHashAddEntry(conn->storagePools, name, ret) < 0) { + if (virHashAddEntry(conn->storagePools, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to add storage pool to connection hash table")); @@ -872,10 +876,12 @@ error: static void virReleaseStoragePool(virStoragePoolPtr pool) { virConnectPtr conn = pool->conn; - DEBUG("release pool %p %s", pool, pool->name); + char uuidstr[VIR_UUID_STRING_BUFLEN]; - /* TODO search by UUID first as they are better differentiators */ - if (virHashRemoveEntry(conn->storagePools, pool->name, NULL) < 0) { + virUUIDFormat(pool->uuid, uuidstr); + DEBUG("release pool %p %s %s", pool, pool->name, uuidstr); + + if (virHashRemoveEntry(conn->storagePools, uuidstr, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("pool missing from connection hash table")); @@ -1421,6 +1427,7 @@ int virUnrefStream(virStreamPtr st) { virNWFilterPtr virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid) { virNWFilterPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -1428,10 +1435,9 @@ virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid) } virMutexLock(&conn->lock); - /* TODO search by UUID first as they are better differentiators */ + virUUIDFormat(uuid, uuidstr); - ret = (virNWFilterPtr) virHashLookup(conn->nwfilterPools, name); - /* TODO check the UUID */ + ret = (virNWFilterPtr) virHashLookup(conn->nwfilterPools, uuidstr); if (ret == NULL) { if (VIR_ALLOC(ret) < 0) { virMutexUnlock(&conn->lock); @@ -1449,7 +1455,7 @@ virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid) if (uuid != NULL) memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); - if (virHashAddEntry(conn->nwfilterPools, name, ret) < 0) { + if (virHashAddEntry(conn->nwfilterPools, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to add network filter pool to connection hash table")); @@ -1485,10 +1491,12 @@ error: static void virReleaseNWFilterPool(virNWFilterPtr pool) { virConnectPtr conn = pool->conn; - DEBUG("release pool %p %s", pool, pool->name); + char uuidstr[VIR_UUID_STRING_BUFLEN]; - /* TODO search by UUID first as they are better differentiators */ - if (virHashRemoveEntry(conn->nwfilterPools, pool->name, NULL) < 0) { + virUUIDFormat(pool->uuid, uuidstr); + DEBUG("release pool %p %s %s", pool, pool->name, uuidstr); + + if (virHashRemoveEntry(conn->nwfilterPools, uuidstr, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", _("pool missing from connection hash table")); -- 1.7.1
2010/6/17 Jiri Denemark <jdenemar@redhat.com>:
Per-connection hashes for domains, networks, storage pools and network filter pools were indexed by names which was not the best choice. UUIDs are better identifiers, so lets use them.
Well, the comments say the same: "UUID is better than the name has key", but why? Because they are more random than the name and might result in less collisions in the hash? Also ID, name and UUID must be unique per domain anyway.
--- src/datatypes.c | 64 +++++++++++++++++++++++++++++++------------------------ 1 files changed, 36 insertions(+), 28 deletions(-)
Anyway, ACK. Matthias
On 06/18/2010 01:08 PM, Matthias Bolte wrote:
2010/6/17 Jiri Denemark <jdenemar@redhat.com>:
Per-connection hashes for domains, networks, storage pools and network filter pools were indexed by names which was not the best choice. UUIDs are better identifiers, so lets use them.
Well, the comments say the same: "UUID is better than the name has key", but why? Because they are more random than the name and might result in less collisions in the hash?
A while ago, there was a bug with libvirt leaking domain references. This caused hash table confusion if a user did: - create VM named 'foobar' - whoops, messed up config. delete VM 'foobar' - create new VM named 'foobar' - createXML errors obscurely Leak fixed, solve the problem, but it shouldn't have caused issues in the first place as long as UUID is different. Granted this change just transfers that possible issue to UUIDs, possible in the case of define $xml; undefine $xmlname; define $xml but that's probably not as useful a usecase. Also keying off UUID might make implementing a rename API easier. - Cole
Also ID, name and UUID must be unique per domain anyway.
--- Â src/datatypes.c | Â 64 +++++++++++++++++++++++++++++++------------------------ Â 1 files changed, 36 insertions(+), 28 deletions(-)
Anyway, ACK.
Matthias
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
The first thing we do in all these functions is to check uuid != NULL and fail if it isn't. --- src/datatypes.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 20b2d74..8750406 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -369,8 +369,7 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { ret->magic = VIR_DOMAIN_MAGIC; ret->conn = conn; ret->id = -1; - if (uuid != NULL) - memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); + memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); ret->snapshots = virHashCreate(20); if (virHashAddEntry(conn->domains, uuidstr, ret) < 0) { @@ -515,8 +514,7 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { } ret->magic = VIR_NETWORK_MAGIC; ret->conn = conn; - if (uuid != NULL) - memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); + memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); if (virHashAddEntry(conn->networks, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); @@ -837,8 +835,7 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui } ret->magic = VIR_STORAGE_POOL_MAGIC; ret->conn = conn; - if (uuid != NULL) - memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); + memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); if (virHashAddEntry(conn->storagePools, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); @@ -1452,8 +1449,7 @@ virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid) } ret->magic = VIR_NWFILTER_MAGIC; ret->conn = conn; - if (uuid != NULL) - memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); + memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); if (virHashAddEntry(conn->nwfilterPools, uuidstr, ret) < 0) { virMutexUnlock(&conn->lock); -- 1.7.1
2010/6/17 Jiri Denemark <jdenemar@redhat.com>:
The first thing we do in all these functions is to check uuid != NULL and fail if it isn't. --- src/datatypes.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index 20b2d74..8750406 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -369,8 +369,7 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { ret->magic = VIR_DOMAIN_MAGIC; ret->conn = conn; ret->id = -1; - if (uuid != NULL) - memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); + memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
We might also get rid of this unnecessary unreferencing and referencing while touching those 3 memcpys and replace memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN); with memcpy(ret->uuid, uuid, VIR_UUID_BUFLEN); ACK. Matthias
We might also get rid of this unnecessary unreferencing and referencing while touching those 3 memcpys and replace
memcpy(&(ret->uuid[0]), uuid, VIR_UUID_BUFLEN);
with
memcpy(ret->uuid, uuid, VIR_UUID_BUFLEN);
That was my thought too. But since git grep gives about 140 of those, it seems it's a candidate for a separate single purpose cleanup patch for all of them. On the other hand, it looks like something a compiler could optimize. Also I wonder if there was a reason for such strange constructs as there are so many of them. Jirka
As anywhere else, uuid is defined as a fixed size array inside _virSecret structure; we shouldn't try to free it. --- src/datatypes.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 8750406..36a2b55 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -1271,7 +1271,6 @@ virGetSecret(virConnectPtr conn, const unsigned char *uuid, error: if (ret != NULL) { VIR_FREE(ret->usageID); - VIR_FREE(ret->uuid); VIR_FREE(ret); } return NULL; -- 1.7.1
2010/6/17 Jiri Denemark <jdenemar@redhat.com>:
As anywhere else, uuid is defined as a fixed size array inside _virSecret structure; we shouldn't try to free it. --- src/datatypes.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index 8750406..36a2b55 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -1271,7 +1271,6 @@ virGetSecret(virConnectPtr conn, const unsigned char *uuid, error: if (ret != NULL) { VIR_FREE(ret->usageID); - VIR_FREE(ret->uuid); VIR_FREE(ret); } return NULL; -- 1.7.1
ACK. Matthias
- Fix documentation for virGetStorageVol: it has 'key' argument instead of 'uuid'. - Remove TODO comment from virReleaseStorageVol: we use volume key as an identifier instead of UUID. - Print human-readable UUID string in debug message in virReleaseSecret. --- src/datatypes.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 36a2b55..46009ae 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -939,7 +939,7 @@ virUnrefStoragePool(virStoragePoolPtr pool) { * @conn: the hypervisor connection * @pool: pool owning the volume * @name: pointer to the storage vol name - * @uuid: pointer to the uuid + * @key: pointer to unique key of the volume * * Lookup if the storage vol is already registered for that connection, * if yes return a new pointer to it, if no allocate a new structure, @@ -1025,7 +1025,6 @@ virReleaseStorageVol(virStorageVolPtr vol) { virConnectPtr conn = vol->conn; DEBUG("release vol %p %s", vol, vol->name); - /* TODO search by UUID first as they are better differentiators */ if (virHashRemoveEntry(conn->storageVols, vol->key, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1291,9 +1290,10 @@ static void virReleaseSecret(virSecretPtr secret) { virConnectPtr conn = secret->conn; char uuidstr[VIR_UUID_STRING_BUFLEN]; - DEBUG("release secret %p %p", secret, secret->uuid); virUUIDFormat(secret->uuid, uuidstr); + DEBUG("release secret %p %s", secret, uuidstr); + if (virHashRemoveEntry(conn->secrets, uuidstr, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.7.1
2010/6/17 Jiri Denemark <jdenemar@redhat.com>:
- Fix documentation for virGetStorageVol: it has 'key' argument instead of 'uuid'. - Remove TODO comment from virReleaseStorageVol: we use volume key as an identifier instead of UUID. - Print human-readable UUID string in debug message in virReleaseSecret. --- src/datatypes.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index 36a2b55..46009ae 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -939,7 +939,7 @@ virUnrefStoragePool(virStoragePoolPtr pool) { * @conn: the hypervisor connection * @pool: pool owning the volume * @name: pointer to the storage vol name - * @uuid: pointer to the uuid + * @key: pointer to unique key of the volume * * Lookup if the storage vol is already registered for that connection, * if yes return a new pointer to it, if no allocate a new structure, @@ -1025,7 +1025,6 @@ virReleaseStorageVol(virStorageVolPtr vol) { virConnectPtr conn = vol->conn; DEBUG("release vol %p %s", vol, vol->name);
- /* TODO search by UUID first as they are better differentiators */ if (virHashRemoveEntry(conn->storageVols, vol->key, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1291,9 +1290,10 @@ static void virReleaseSecret(virSecretPtr secret) { virConnectPtr conn = secret->conn; char uuidstr[VIR_UUID_STRING_BUFLEN]; - DEBUG("release secret %p %p", secret, secret->uuid);
virUUIDFormat(secret->uuid, uuidstr); + DEBUG("release secret %p %s", secret, uuidstr); + if (virHashRemoveEntry(conn->secrets, uuidstr, NULL) < 0) { virMutexUnlock(&conn->lock); virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.7.1
ACK. Matthias
participants (3)
-
Cole Robinson -
Jiri Denemark -
Matthias Bolte