[Insight-developers] Patch for gdcm::Util::CreateUniqueUID

[Mathieu Malaterre]

On Mon, Apr 21, 2008 at 3:08 PM, Henning Meyer <tutmann at gmail.com> wrote:
> Hello All,
>
>  I wrote a small Patch for gdcm::Util::CreateUniqueUID, because with my
>  setup I had UID-Clashes under Windows from time to time.
>  I think its a good idea to use a Counter instead of a two-digit random
>  number and also to include thread-id or process-id.
>  I hope I won't offend you to much by not using the version control,
>  but this seemed easiest.
>  I hope you like it and include it to the repository.

Hi Henning,

Here are a couple of comments on your patch:

+
+// taken from http://de.wikipedia.org/wiki/FNV_%28Informatik%29
+unsigned long long fnFNV( const void* pBuffer, size_t nByteLen ) {
+	unsigned long long nHashVal    = 0x84222325cbf29ce4ULL;

where is this hash value coming from ? See my comment on the wikipedia
talk page:

http://de.wikipedia.org/wiki/Diskussion:FNV_%28Informatik%29

I believe this value is incorrect (or at least different from the fnv
1a implementation).

+	unsigned long long nMagicPrime = 0x00000100000001b3ULL;
+
+	unsigned char* pFirst = ( unsigned char* )( pBuffer );
+	unsigned char* pLast  = pFirst + nByteLen;
+
+	while( pFirst < pLast ) {
+		nHashVal ^= *pFirst++;
+		nHashVal *= nMagicPrime;
+	}
+	return nHashVal;
+}
+

The following code is win32 only. For instance using the
pthread_self() equivalent code on my linux box lead to a threadid with
approx 10 digits. This is way too long to be stored as is.

+   char tmp[10];
+   sprintf(tmp,"%d", GetCurrentThreadID());
+   append += ".";
+   append += tmp;
+   sprintf(tmp,"%d", GetCurrentProcessID());
+   append += ".";
+   append += tmp;
+   static unsigned int counter = 0;
+   ++counter;
+   sprintf(tmp,"%d", counter);
+   append += ".";
+   append += tmp;

I do not like the counter since it never get reset. You expect the
number to wrap up at some point, but the process id and potentially
the thread id will be the same at this point. The number will get too
big anyway and cannot be stored as is.

+   std::string result;
+   const unsigned int hashbits = 64;
+   const unsigned int hashlength = 1+(unsigned int)(hashbits *
0.30102999566398119521373889472449 /* log(2) */);
+   if (prefix.size() > 64-hashlength) {
+		result = hash_string( prefix + append );

That's completely illegal ! When an authority gives you this prefix,
you are supposed to use that prefix and nothing else !

+   } else {
+		result = prefix + hash_string( append );
+   }

Please make sure that all your generated image have:
1. The proper prefix
2. A UID of at most 64 bytes


I have sent some further comments (to itk-dev a couple of minutes
ago), unless you have some reasons to think my proposed approach is
wrong, your patch will not make it into gdcm, and instead I'll use an
existing implementation of UUID generator. Technically

* UuidCreate on Win32 system
* uuid_create on *BSD system
* and uuid_generate on other *nix system

Thanks,
-- 
Mathieu