"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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?
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list