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

"Daniel P. Berrange" <berrange(a)redhat.com&gt; 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?