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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 30, 2018 at 03:18:04PM +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).
> 
> It uses the vif index to account for multiple vifs.
> 
> MD5 code is originally from the public domain (written by Colin Plumb in
> 1993), files found in xen/tools/blktap2/drivers/.
> 
> Reported-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Joshua Perrett <jperrett256@xxxxxxxxx>
> ---
>  tools/libxl/Makefile    |   2 +-
>  tools/libxl/libxl_nic.c |  65 ++++++++++--
>  tools/libxl/md5.c       | 266 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/md5.h       |  26 +++++
>  4 files changed, 352 insertions(+), 7 deletions(-)
>  create mode 100644 tools/libxl/md5.c
>  create mode 100644 tools/libxl/md5.h
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 6da342ed61..6e7db11367 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -142,7 +142,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o 
> libxl_dm.o libxl_pci.o \
>                       libxl_9pfs.o libxl_domain.o libxl_vdispl.o \
>                       libxl_pvcalls.o libxl_vsnd.o libxl_vkb.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
> -LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
> +LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o md5.o
>  
>  LIBXL_TESTS += timedereg
>  LIBXL_TESTS_PROGS = $(LIBXL_TESTS) fdderegrace
> diff --git a/tools/libxl/libxl_nic.c b/tools/libxl/libxl_nic.c
> index 01b711b84e..9a1c0fb16f 100644
> --- a/tools/libxl/libxl_nic.c
> +++ b/tools/libxl/libxl_nic.c
> @@ -17,6 +17,18 @@
>  
>  #include "libxl_internal.h"
>  
> +#include <string.h>
> +
> +#include "md5.h"
> +
> +#ifdef __linux__
> +#include <sys/types.h>
> +#include <ifaddrs.h>
> +#include <sys/socket.h>
> +#include <linux/if_packet.h>
> +#include <net/ethernet.h>
> +#endif
> +
>  int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>                              const char *mac, libxl_device_nic *nic)
>  {
> @@ -53,8 +65,38 @@ int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>      return rc;
>  }
>  
> +static int libxl__get_host_mac(libxl__gc *gc, unsigned char *buf)
> +{
> +    int rc = -1;

rc should be initialised to ERROR_FAIL;

> +    #ifdef __linux__
> +    struct ifaddrs *iface_list;
> +
> +    if (getifaddrs(&iface_list) == 0) {

You haven't answered my question regarding stability of this list.

> +        for (struct ifaddrs *iface = iface_list;
> +            iface != NULL; iface = iface->ifa_next) {
> +            if (iface->ifa_addr && iface->ifa_addr->sa_family == AF_PACKET) {
> +                struct sockaddr_ll *s = (struct sockaddr_ll 
> *)iface->ifa_addr;
> +                if (s->sll_halen == 6) {
> +                    memcpy(buf, s->sll_addr, 6);
> +                    if(buf[0] || buf[1] || buf[2] || buf[3] || buf[4] || 
> buf[5]) {

Missing space after if.

Also please wrap the line around 80 characters.


> +                        rc = 0;
> +                        break;
> +                    }
> +                }
> +            }
> +        }
> +        freeifaddrs(iface_list);
> +    } else {
> +        LOG(ERROR, "getifaddrs\n");

If this is not a fatal error you should use WARN here.

> +    }
> +    #endif
> +
> +    return rc;
> +}
> +
>  static int libxl__device_nic_setdefault(libxl__gc *gc, uint32_t domid,
> -                                        libxl_device_nic *nic, bool hotplug)
> +                                        libxl_device_nic *nic, const char 
> *name,
> +                                        const int nic_index, bool hotplug)
>  {
>      int rc;
>  
> @@ -65,11 +107,22 @@ static int libxl__device_nic_setdefault(libxl__gc *gc, 
> uint32_t domid,
>          if (!nic->model) return ERROR_NOMEM;
>      }
>      if (libxl__mac_is_default(&nic->mac)) {
> -        const uint8_t *r;
> -        libxl_uuid uuid;
> +        uint8_t r[16];
> +
> +        MD5_CTX ctx;
> +        MD5Init(&ctx);
> +
> +        uint8_t hostmac[6] = {0};
> +
> +        if(libxl__get_host_mac(gc, hostmac) == 0) {
> +            MD5Update(&ctx, hostmac, sizeof(hostmac));
> +        } else {
> +            LOGD(WARN, domid, "failed to get host mac address, will generate 
> vm mac address without\n");

Turn WARN into INFO.

Wei.

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