3:56 p.m.
With the previous storage pool UUID source not all storage pools
had a proper UUID, especially GSX storage pools. The mount path
is unique per host and cannot change during the lifetime of the
datastore. Therefore, its MD5 sum can be used as UUID.
Use gnulib's crypto/md5 module to generate the MD5 sum.
---
bootstrap.conf | 1 +
src/esx/esx_storage_driver.c | 107 +++++++++++-------------------------------
2 files changed, 28 insertions(+), 80 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf
index 5af77a7..ca31a6e 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -26,6 +26,7 @@ canonicalize-lgpl
close
connect
count-one-bits
+crypto/md5
dirname-lgpl
fcntl-h
getaddrinfo
diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c
index 644e66c..adbe8c6 100644
--- a/src/esx/esx_storage_driver.c
+++ b/src/esx/esx_storage_driver.c
@@ -24,6 +24,7 @@
#include <config.h>
+#include "md5.h"
#include "internal.h"
#include "util.h"
#include "memory.h"
@@ -197,10 +198,7 @@ esxStoragePoolLookupByName(virConnectPtr conn, const char *name)
esxPrivate *priv = conn->storagePrivateData;
esxVI_ObjectContent *datastore = NULL;
esxVI_DatastoreHostMount *hostMount = NULL;
- char *suffix = NULL;
- int suffixLength;
- char uuid_string[VIR_UUID_STRING_BUFLEN] =
"00000000-00000000-0000-000000000000";
- unsigned char uuid[VIR_UUID_BUFLEN];
+ unsigned char md5[MD5_DIGEST_SIZE]; /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
virStoragePoolPtr pool = NULL;
if (esxVI_EnsureSession(priv->primary) < 0) {
@@ -213,53 +211,22 @@ esxStoragePoolLookupByName(virConnectPtr conn, const char *name)
}
/*
- * Datastores don't have a UUID. We can use the 'host.mountInfo.path'
- * property as source for a "UUID" on ESX, because the property value has
- * this format:
+ * Datastores don't have a UUID, but we can use the
'host.mountInfo.path'
+ * property as source for a UUID. The mount path is unique per host and
+ * cannot change during the lifetime of the datastore.
*
- * host.mountInfo.path = /vmfs/volumes/4b0beca7-7fd401f3-1d7f-000ae484a6a3
- * host.mountInfo.path = /vmfs/volumes/b24b7a78-9d82b4f5 (short format)
- *
- * The 'host.mountInfo.path' property comes in two forms, with a complete
- * "UUID" and a short "UUID".
- *
- * But this trailing "UUID" is not guaranteed to be there. On the other
- * hand we already rely on another implementation detail of the ESX server:
- * The object name of virtual machine contains an integer, we use that as
- * domain ID.
+ * The MD5 sum of the mount path can be used as UUID, assuming MD5 is
+ * considered to be collision-free enough for this use case.
*/
if (esxVI_LookupDatastoreHostMount(priv->primary, datastore->obj,
&hostMount) < 0) {
goto cleanup;
}
- if ((suffix = STRSKIP(hostMount->mountInfo->path, "/vmfs/volumes/"))
!= NULL) {
- suffixLength = strlen(suffix);
+ md5_buffer(hostMount->mountInfo->path,
+ strlen(hostMount->mountInfo->path), md5);
- if ((suffixLength == 35 && /* =
strlen("4b0beca7-7fd401f3-1d7f-000ae484a6a3") */
- suffix[8] == '-' && suffix[17] == '-' &&
suffix[22] == '-') ||
- (suffixLength == 17 && /* = strlen("b24b7a78-9d82b4f5") */
- suffix[8] == '-')) {
- /*
- * Intentionally use memcpy here, because we want to be able to
- * replace a prefix of the initial Zero-UUID. virStrncpy would
- * null-terminate the string in an unwanted place.
- */
- memcpy(uuid_string, suffix, suffixLength);
- } else {
- VIR_WARN("Datastore host mount path suffix '%s' has unexpected
"
- "format, cannot deduce a UUID from it", suffix);
- }
- }
-
- if (virUUIDParse(uuid_string, uuid) < 0) {
- ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse UUID from string '%s'"),
- uuid_string);
- goto cleanup;
- }
-
- pool = virGetStoragePool(conn, name, uuid);
+ pool = virGetStoragePool(conn, name, md5);
cleanup:
esxVI_ObjectContent_Free(&datastore);
@@ -275,9 +242,11 @@ esxStoragePoolLookupByUUID(virConnectPtr conn, const unsigned char
*uuid)
{
esxPrivate *priv = conn->storagePrivateData;
esxVI_String *propertyNameList = NULL;
+ esxVI_ObjectContent *datastoreList = NULL;
esxVI_ObjectContent *datastore = NULL;
+ esxVI_DatastoreHostMount *hostMount = NULL;
+ unsigned char md5[MD5_DIGEST_SIZE]; /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
char uuid_string[VIR_UUID_STRING_BUFLEN] = "";
- char *absolutePath = NULL;
char *name = NULL;
virStoragePoolPtr pool = NULL;
@@ -285,48 +254,26 @@ esxStoragePoolLookupByUUID(virConnectPtr conn, const unsigned char
*uuid)
return NULL;
}
- /*
- * Convert UUID to 'host.mountInfo.path' form by stripping the second
'-':
- *
- * <---- 14 ----><-------- 22 --------> <---- 13
---><-------- 22 -------->
- * 4b0beca7-7fd4-01f3-1d7f-000ae484a6a3 -> 4b0beca7-7fd401f3-1d7f-000ae484a6a3
- */
- virUUIDFormat(uuid, uuid_string);
- memmove(uuid_string + 13, uuid_string + 14, 22 + 1);
-
- if (virAsprintf(&absolutePath, "/vmfs/volumes/%s", uuid_string) < 0)
{
- virReportOOMError();
- goto cleanup;
- }
-
if (esxVI_String_AppendValueToList(&propertyNameList, "summary.name")
< 0 ||
- esxVI_LookupDatastoreByAbsolutePath(priv->primary, absolutePath,
- propertyNameList, &datastore,
- esxVI_Occurrence_OptionalItem) < 0) {
+ esxVI_LookupDatastoreList(priv->primary, propertyNameList,
+ &datastoreList) < 0) {
goto cleanup;
}
- /*
- * If the first try didn't succeed and the trailing 16 digits are zero then
- * the "UUID" could be a short one. Strip the 16 zeros and try again:
- *
- * <------ 17 -----> <------ 17 ----->
- * b24b7a78-9d82b4f5-0000-000000000000 -> b24b7a78-9d82b4f5
- */
- if (datastore == NULL && STREQ(uuid_string + 17,
"-0000-000000000000")) {
- uuid_string[17] = '\0';
+ for (datastore = datastoreList; datastore != NULL;
+ datastore = datastore->_next) {
+ esxVI_DatastoreHostMount_Free(&hostMount);
- VIR_FREE(absolutePath);
-
- if (virAsprintf(&absolutePath, "/vmfs/volumes/%s", uuid_string)
< 0) {
- virReportOOMError();
+ if (esxVI_LookupDatastoreHostMount(priv->primary, datastore->obj,
+ &hostMount) < 0) {
goto cleanup;
}
- if (esxVI_LookupDatastoreByAbsolutePath(priv->primary, absolutePath,
- propertyNameList, &datastore,
- esxVI_Occurrence_RequiredItem) < 0) {
- goto cleanup;
+ md5_buffer(hostMount->mountInfo->path,
+ strlen(hostMount->mountInfo->path), md5);
+
+ if (memcmp(uuid, md5, VIR_UUID_BUFLEN) == 0) {
+ break;
}
}
@@ -348,9 +295,9 @@ esxStoragePoolLookupByUUID(virConnectPtr conn, const unsigned char
*uuid)
pool = virGetStoragePool(conn, name, uuid);
cleanup:
- VIR_FREE(absolutePath);
esxVI_String_Free(&propertyNameList);
- esxVI_ObjectContent_Free(&datastore);
+ esxVI_ObjectContent_Free(&datastoreList);
+ esxVI_DatastoreHostMount_Free(&hostMount);
return pool;
}
--
1.7.0.4
4:55 a.m.
On 08/09/2010 06:56 AM, Matthias Bolte wrote:
With the previous storage pool UUID source not all storage pools
had a proper UUID, especially GSX storage pools. The mount path
is unique per host and cannot change during the lifetime of the
datastore. Therefore, its MD5 sum can be used as UUID.
Use gnulib's crypto/md5 module to generate the MD5 sum.
Just as a general thought, would it be preferably to use the SHA1
function instead of the MD5 one?
Asking because MD5 seems to be trending "out of favour" (as a
generalisation) compared to SHA1, after some cryptographers showed MD5
hash collisions could be practically achieved with some types of data.
(or something along those lines)
For all intents and purposes, I'm thinking it'll make no practical
functional difference, but figure it's worth asking. :)
Regards and best wishes,
Justin Clift
6:40 p.m.
2010/8/9 Justin Clift <jclift(a)redhat.com>:
On 08/09/2010 06:56 AM, Matthias Bolte wrote:
>
> With the previous storage pool UUID source not all storage pools
> had a proper UUID, especially GSX storage pools. The mount path
> is unique per host and cannot change during the lifetime of the
> datastore. Therefore, its MD5 sum can be used as UUID.
>
> Use gnulib's crypto/md5 module to generate the MD5 sum.
Just as a general thought, would it be preferably to use the SHA1 function
instead of the MD5 one?
Every non-trivial hash function will do for the purpose of deriving a
UUID from the mount path.
Asking because MD5 seems to be trending "out of favour" (as
a
generalisation) compared to SHA1, after some cryptographers showed MD5 hash
collisions could be practically achieved with some types of data. (or
something along those lines)
I'm aware of MD5 being considered as broken for cryptographic use
cases, but this one isn't cryptographic so I don't care about the
problem of crafted collisions here. The only real problem here would
be when you have two datastores with different mount paths that hash
to the same UUID, but I think md5 is collision-free enough so we can
ignore this problem.
There is another "problem" with the approach of using the mount path
hash as UUID. In case of a Windows based GSX server you can use CIFS
shares as datastores addressed via UNC paths like \\nas\share. If you
have multiple GSX server sharing that datastore then all will have the
same UUID because the have the same mount path. Actually I don't
consider this as a real problem because in this case there is one
datastore with one UUID shared between multiple hosts.
For all intents and purposes, I'm thinking it'll make no
practical
functional difference, but figure it's worth asking. :)
gnulib also provides SHA1 and SHA256 so we could use those instead of
MD5, but I think MD5 is good enough for this special use case here.
Matthias
10:45 p.m.
New subject: [libvirt] [PATCH] esx: Use MD5 sum of mount path as storage pool UUID
Anno domini 2010 Matthias Bolte scripsit:
[...]
There is another "problem" with the approach of using the
mount path
hash as UUID. In case of a Windows based GSX server you can use CIFS
shares as datastores addressed via UNC paths like \\nas\share. If you
have multiple GSX server sharing that datastore then all will have the
same UUID because the have the same mount path. Actually I don't
consider this as a real problem because in this case there is one
datastore with one UUID shared between multiple hosts.
Would this become a problem if the user would use \\nas one node and
\\nas.dom.aim on another node? The nasty thing about network paths is
that people might use aliases..
This just came to my mind.
Ciao
Max
--
Gib Dein Bestes. Dann übertriff Dich selbst!
6:07 p.m.
2010/8/12 Maximilian Wilhelm <max(a)rfc2324.org>:
Anno domini 2010 Matthias Bolte scripsit:
[...]
> There is another "problem" with the approach of using the mount path
> hash as UUID. In case of a Windows based GSX server you can use CIFS
> shares as datastores addressed via UNC paths like \\nas\share. If you
> have multiple GSX server sharing that datastore then all will have the
> same UUID because the have the same mount path. Actually I don't
> consider this as a real problem because in this case there is one
> datastore with one UUID shared between multiple hosts.
Would this become a problem if the user would use \\nas one node and
\\nas.dom.aim on another node? The nasty thing about network paths is
that people might use aliases..
This just came to my mind.
Ciao
Max
It doesn't affect the per-connection UUID-to-datastore mapping, that's
the important part.
It will result in different UUIDs for the same datastore, so it
affects the global datastore-to-UUID mapping, but that's okay, there's
nothing the driver could do about that.
The important point is that every datastore has a UUID now and that
this UUID is at least unique per connection.
Matthias
11:02 p.m.
On Thu, Aug 12, 2010 at 01:40:14AM +0200, Matthias Bolte wrote:
2010/8/9 Justin Clift <jclift(a)redhat.com>:
> On 08/09/2010 06:56 AM, Matthias Bolte wrote:
>>
>> With the previous storage pool UUID source not all storage pools
>> had a proper UUID, especially GSX storage pools. The mount path
>> is unique per host and cannot change during the lifetime of the
>> datastore. Therefore, its MD5 sum can be used as UUID.
>>
>> Use gnulib's crypto/md5 module to generate the MD5 sum.
>
> Just as a general thought, would it be preferably to use the SHA1 function
> instead of the MD5 one?
Every non-trivial hash function will do for the purpose of deriving a
UUID from the mount path.
> Asking because MD5 seems to be trending "out of favour" (as a
> generalisation) compared to SHA1, after some cryptographers showed MD5 hash
> collisions could be practically achieved with some types of data. (or
> something along those lines)
I'm aware of MD5 being considered as broken for cryptographic use
cases, but this one isn't cryptographic so I don't care about the
problem of crafted collisions here. The only real problem here would
be when you have two datastores with different mount paths that hash
to the same UUID, but I think md5 is collision-free enough so we can
ignore this problem.
There is another "problem" with the approach of using the mount path
hash as UUID. In case of a Windows based GSX server you can use CIFS
shares as datastores addressed via UNC paths like \\nas\share. If you
have multiple GSX server sharing that datastore then all will have the
same UUID because the have the same mount path. Actually I don't
consider this as a real problem because in this case there is one
datastore with one UUID shared between multiple hosts.
This is actually a feature! If the same storage pool is visible on
multiple hosts, it is absolutely desirable that it have the same
UUID on each host.
> For all intents and purposes, I'm thinking it'll make no
practical
> functional difference, but figure it's worth asking. :)
>
gnulib also provides SHA1 and SHA256 so we could use those instead of
MD5, but I think MD5 is good enough for this special use case here.
Agreed, we don't need cryptographic security for this use case, so MD5
is fine.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:45 a.m.
On 08/08/2010 02:56 PM, Matthias Bolte wrote:
With the previous storage pool UUID source not all storage pools
had a proper UUID, especially GSX storage pools. The mount path
is unique per host and cannot change during the lifetime of the
datastore. Therefore, its MD5 sum can be used as UUID.
Use gnulib's crypto/md5 module to generate the MD5 sum.
It looks like others have agreed on the approach; but I didn't see an
actual ACK. So just in case:
@@ -197,10 +198,7 @@ esxStoragePoolLookupByName(virConnectPtr conn,
const char *name)
esxPrivate *priv = conn->storagePrivateData;
esxVI_ObjectContent *datastore = NULL;
esxVI_DatastoreHostMount *hostMount = NULL;
- char *suffix = NULL;
- int suffixLength;
- char uuid_string[VIR_UUID_STRING_BUFLEN] =
"00000000-00000000-0000-000000000000";
- unsigned char uuid[VIR_UUID_BUFLEN];
+ unsigned char md5[MD5_DIGEST_SIZE]; /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
Is it worth also adding:
#include "verify.h"
...
verify(MD5_DIGEST_SIZE == VIR_UUID_BUFLEN)
to enforce this comment's correctness?
ACK with that nit addressed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:32 p.m.
2010/8/14 Eric Blake <eblake(a)redhat.com>:
On 08/08/2010 02:56 PM, Matthias Bolte wrote:
> With the previous storage pool UUID source not all storage pools
> had a proper UUID, especially GSX storage pools. The mount path
> is unique per host and cannot change during the lifetime of the
> datastore. Therefore, its MD5 sum can be used as UUID.
>
> Use gnulib's crypto/md5 module to generate the MD5 sum.
It looks like others have agreed on the approach; but I didn't see an
actual ACK. So just in case:
> @@ -197,10 +198,7 @@ esxStoragePoolLookupByName(virConnectPtr conn, const char
*name)
> esxPrivate *priv = conn->storagePrivateData;
> esxVI_ObjectContent *datastore = NULL;
> esxVI_DatastoreHostMount *hostMount = NULL;
> - char *suffix = NULL;
> - int suffixLength;
> - char uuid_string[VIR_UUID_STRING_BUFLEN] =
"00000000-00000000-0000-000000000000";
> - unsigned char uuid[VIR_UUID_BUFLEN];
> + unsigned char md5[MD5_DIGEST_SIZE]; /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16
*/
Is it worth also adding:
#include "verify.h"
...
verify(MD5_DIGEST_SIZE == VIR_UUID_BUFLEN)
to enforce this comment's correctness?
ACK with that nit addressed.
That's a good idea. I've folded that in and pushed the result.
Matthias
5238
days inactive
5250
days old
7 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Justin Clift -
Matthias Bolte -
Maximilian Wilhelm