Re: random number stuff

[Tim Janik <timj gtk org>]

On Thu, 18 Dec 2003, George wrote:

> Sorry it took so long, even being such a simple patch.  In any case
> bug filed, patch attached to the bug see:
> http://bugzilla.gnome.org/show_bug.cgi?id=129636
>
> Added functions
> g_rand_new_with_seed_array
> g_rand_set_seed_array
> g_rand_copy
>
> Patch has a ChangeLog
>
> Also added some testing to rand-test.c to see that it works just like the
> reference implementation.


hi george, a couple comments on your patch:

> +  switch (get_random_version ())
> +    {
> +    case 20:
> +      /* array seeds are not tested/tuned for 2.0 version of
> +	 the seeding, so just seed with a guint32 taking into
> +	 account that seed[0] can be secs and seed[1] usecs */
> +      return g_rand_new_with_seed ((seed[0]<<20) ^ seed[1]);
> +
> +      break;
> +    case 22:
> +      return g_rand_new_with_seed_array (seed, 4);
> +
> +      break;
> +    default:
> +      g_assert_not_reached ();
>      }

there's no need to special case get_random_version() in g_rand_new(), the
2.0 version was (meant to be) unpredictable anyways, so you can always use
g_rand_new_with_seed_array() here.

and a comment on the initialization code currently in CVS (which your patch
doesn't change):

> #ifdef G_OS_UNIX
>   static gboolean dev_urandom_exists = TRUE;
>
>   if (dev_urandom_exists)
>     {
>       FILE* dev_urandom = fopen("/dev/urandom", "rb");
>       if (dev_urandom)
>         {
>           if (fread (&seed, sizeof (seed), 1, dev_urandom) != 1)
>             dev_urandom_exists = FALSE;
>           fclose (dev_urandom);

this should at least check errno, to avoid things like EINTR
due to a signal causing future initializations to bypass /dev/urandom.

>         }
>       else
>         dev_urandom_exists = FALSE;
>     }
> #else


and back to your patch:

> @@ -156,10 +175,31 @@ g_rand_new (void)
>    if (!dev_urandom_exists)
>      {
>        g_get_current_time (&now);
> -      seed = now.tv_sec ^ now.tv_usec;
> +      seed[0] = now.tv_sec;
> +      seed[1] = now.tv_usec;
> +      seed[2] = getpid ();
> +      seed[3] = getppid ();
> +    }
> +

with array seeding, even if we have /dev/urandom, it wouldn't
hurt to add up the secs/usecs unconditionally.


>  /**
> + * g_rand_set_seed_array:
> + * @rand_: a #GRand.
> + * @seed: array to initialize with
> + * @seed_length: length of array
> + *
> + * Initializes the random number generator by an array of
> + * longs.  Array can be of arbitrary size, though only the
> + * first 624 values are taken.  Only works properly
> + * if using the 2.2 random version.  If you are using the 2.0
> + * version, results may not be optimal.  This function is useful
> + * if you have many low entropy seeds, or if you require more then
> + * 32bits of actual entropy for your application.
> + **/

you can remove the blurb about using the 2.0 version. the version special
casing affects:
1) optimum entropy usage of the seed value in g_random_seed(), but
   g_rand_set_seed_array() doesn't have that weakness.
2) uniformness of distribution in g_rand_int_range(). that's orthogonal
   to using g_rand_set_seed_array() vs. g_random_seed() though.


those items fixed, the patch looks good to me, and i think
we should get that in before the freeze.

>
> George
>

---
ciaoTJ