Re: [libvirt] PATCH 1/4: More generic MAC address handling
"Daniel P. Berrange" <berrange(a)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?