Re: [Qemu-devel] [PATCH] linux-user: Let user specify random seed

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

On 10/09/2014 02:36 AM, Magnus Reftel wrote:
> This patch introduces the -seed command line option and the
> QEMU_RAND_SEED environment variable for setting the random seed, which
> is used for the AT_RANDOM ELF aux entry.
> 
> Signed-off-by: Magnus Reftel <reftel@spotify.com>
> ---

>  
> +static void handle_arg_randseed(const char *arg)
> +{
> +    unsigned long seed;
> +    char* end;

Style: we prefer:

char *end;

> +    seed = strtoul(arg, &end, 0);
> +    if (end==arg || *end!='\0' || seed > UINT_MAX) {

Style: spaces around operators:

if (end == arg || *end || seed > UINT_MAX) {

Bug: strtoul() sometimes reports error via errno; the only safe way to
use it is to first prime errno = 0, then do strtoul, then check if errno
was changed.

Reimplementation: util/cutils.c already provides parse_uint() that takes
care of calling strtoul safely (hmm, that version only parses 64-bit
numbers; maybe we should expand it to also parse 32-bit numbers?)

Surprising behavior: your code behaves differently on 32-bit hosts than
it does on 64-bit hosts.  Seriously.  strotoul() has the annoying
specification of requiring twos-complement wraparound according to the
size of long, which means "-1" on a 32-bit platform parses as 0xffffffff
(accepted), while on a 64-bit platform parses it as 0xffffffffffffffff
(which you reject as > UINT_MAX); conversely "-18446744073709551615"
fails to parse due to overflow on a 32-bit platform, while successfully
being parsed as 1 on 64-bit.

> +        fprintf(stderr, "Invalid seed number: %s\n", arg);
> +        exit(1);
> +    }
> +    srand(seed);
> +}
> +
>  static void handle_arg_gdb(const char *arg)
>  {
>      gdbstub_port = atoi(arg);
> @@ -3674,6 +3686,8 @@ static const struct qemu_argument arg_table[] = {
>       "",           "run in singlestep mode"},
>      {"strace",     "QEMU_STRACE",      false, handle_arg_strace,
>       "",           "log system calls"},
> +    {"seed",       "QEMU_RAND_SEED",   true,  handle_arg_randseed,
> +     "",           "Seed for pseudo-random number generator"},
>      {"version",    "QEMU_VERSION",     false, handle_arg_version,
>       "",           "display version information and exit"},
>      {NULL, NULL, false, NULL, NULL, NULL}
> @@ -3856,6 +3870,8 @@ int main(int argc, char **argv, char **envp)
>      cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
>  #endif
>  
> +    srand(time(NULL));
> +
>      optind = parse_args(argc, argv);
>  
>      /* Zero out regs */
> @@ -3926,6 +3942,10 @@ int main(int argc, char **argv, char **envp)
>          do_strace = 1;
>      }
>  
> +    if (getenv("QEMU_RAND_SEED")) {
> +        handle_arg_randseed(getenv("QEMU_RAND_SEED"));
> +    }

Now that you have exactly one caller of the static function, it might
make sense to just inline the body of that function here.

> +
>      target_environ = envlist_to_environ(envlist, NULL);
>      envlist_free(envlist);
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]