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

[Daniel P. Berrange]

On Thu, Oct 16, 2008 at 12:37:36PM +0200, Jim Meyering wrote:
> "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.

This was just moving code from elsewhere, not sure why it was like that
in the first place. Seems sensible to just remove the '1 +' bit.

> 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?

Could just add a call to srand() in virInitialize() i guess, although is
that a reasonable thing for a shared library to be doing, rather than 
leaving it upto the application ? 

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|