"Daniel P. Berrange" <berrange@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?