Re: [PATCH] libxl: made vm mac address assignment deterministic

[Ian Jackson <ian.jackson@citrix.com>]

Wei Liu writes ("Re: [PATCH] libxl: made vm mac address assignment deterministic"):
> On Wed, Sep 05, 2018 at 12:25:55PM +0000, Joshua Perrett wrote:
> > Uses MD5 on the host mac address, vm name and vif index to generate the
> > last three bytes of the vm mac address (for each vm).

There is no such thing as "the" host mac address.  The host might
have several.  However, generally there is a specific interface that
will be used for this guest, depending on the vif connection mode.  In
bridge mode, for example, there is the mac address of the bridge.  I
think you should make sure to use the right interface.

I think you need to add something to the documentation.  You should
mention that this approach is only deterministic *on the same host*
(so in setups where the guest might be started on multiple hosts, with
networked storage, it won't work) and only *with the same physical
nic* (so swapping out the physical nic will change all the guests'
addresses).

> > MD5 code is originally from the public domain (written by Colin Plumb in
> > 1993), files found in xen/tools/blktap2/drivers/.

You are duplicating these files within the Xen tree.  That's pretty
ugly.  Can we not do this a better way ?

> > +static int libxl__get_host_mac(libxl__gc *gc, unsigned char *buf)
> > +{
> > +    int rc = ERROR_FAIL;
> 
> Having a blank line makes things a bit clearer.
> 
> > +    #ifdef __linux__
> > +    struct ifaddrs *iface_list;

Can we not have #ifdefs here please ?  The Linux-specific part should
go in libxl_linux.c.

> > +                    memcpy(&value, s->sll_addr, 6);
> 
> Please use sizeof(s->sll_addr) instead of 6.

Err, are you sure ?  Let me quote the proposed code:

> > +                if (s->sll_halen == 6) {
> > +                    uint64_t value = 0;
> > +                    memcpy(&value, s->sll_addr, 6);

This is very strange.  How can it possibly be right to overwrite the
first 6 bytes of value ?  That does a different thing on big-endian
and little-endian machines.  (

Also why are we not checking sll_hatype ?

> > +                    if (value > largest) {
> > +                        memcpy(buf, s->sll_addr, 6);
> > +                        largest = value;
> > +                        rc = 0;
> 
> Interesting algorithm, but I don't know anything better at the moment.
> :)

There is some reason for choosing the lexically largest address, IIRC.
But the reason should be in a comment in the code.  (ISTR reading
something about this in the Debian bridge-interfaces(5) manpage.)

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel