[libvirt] PATCH 1/4: More generic MAC address handling

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> This patch improves the MAC address handling.
>
> Currently our XML parser auto-generates a MAC addres using the KVM vendor
> prefix. This isn't much use for other drivers. This patch addresses this:
>
>  - Stores each driver's vendor prefix in the capability object
>  - Changes domain parser to use the per-driver vendor prefix for
>    generating mac addresses
>  - Adds more utility methods to util.c for parsing/generating/formatting
>    MAC addresses
>  - Updates each driver to record its vendor prefix for MAC address.

This all looks fine, but I have a question about the moved code
that makes up the new virGenerateMacAddr function:

> +void virGenerateMacAddr(const unsigned char *prefix,
> +                        unsigned char *addr)
> +{
> +    addr[0] = prefix[0];
> +    addr[1] = prefix[1];
> +    addr[2] = prefix[2];
> +    addr[3] = 1 + (int)(256*(rand()/(RAND_MAX+1.0)));
> +    addr[4] = 1 + (int)(256*(rand()/(RAND_MAX+1.0)));
> +    addr[5] = 1 + (int)(256*(rand()/(RAND_MAX+1.0)));
> +}

I presume the intent is to generated numbers in 0..255,
but that code generates them in 1..256 and relies on the
truncate-to-8-bit-unsigned-char to map back to 0..255.

Why are those "1 +" there?
It doesn't seem to hurt, but doesn't make sense (to me), either.
Removing them would not change the results.

Also, unless there's a guarantee that the random number state is
initialized elsewhere, it should be initialized here, like it was in
the now-removed xenXMAutoAssignMac function.

       srand((unsigned)time(NULL));

Or maybe just initialize it once at start-up?