9:45 a.m.
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(-)
9:45 a.m.
New subject: [libvirt] [PATCH 1/4] Index hashes by UUID instead of name
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
12:08 p.m.
New subject: [libvirt] [PATCH 1/4] Index hashes by UUID instead of name
2010/6/17 Jiri Denemark <jdenemar(a)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
1:33 p.m.
New subject: [libvirt] [PATCH 1/4] Index hashes by UUID instead of name
On 06/18/2010 01:08 PM, Matthias Bolte wrote:
2010/6/17 Jiri Denemark <jdenemar(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:45 a.m.
New subject: [libvirt] [PATCH 2/4] Remove unnecessary check for non-NULL uuid
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
12:30 p.m.
New subject: [libvirt] [PATCH 2/4] Remove unnecessary check for non-NULL uuid
2010/6/17 Jiri Denemark <jdenemar(a)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
4:43 a.m.
New subject: [libvirt] [PATCH 2/4] Remove unnecessary check for non-NULL uuid
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
9:45 a.m.
New subject: [libvirt] [PATCH 3/4] Do not free static buffer with UUID
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
12:33 p.m.
New subject: [libvirt] [PATCH 3/4] Do not free static buffer with UUID
2010/6/17 Jiri Denemark <jdenemar(a)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
9:45 a.m.
New subject: [libvirt] [PATCH 4/4] Misc cleanups
- 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
12:34 p.m.
New subject: [libvirt] [PATCH 4/4] Misc cleanups
2010/6/17 Jiri Denemark <jdenemar(a)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
5269
days inactive
5273
days old
11 comments
3 participants
participants (3)
-
Cole Robinson -
Jiri Denemark -
Matthias Bolte