|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libxl: made vm mac address assignment deterministic
Also CC George
We had a race -- I commented on your patch on github. I will redo my
review here.
On Wed, Aug 29, 2018 at 04:34:28PM +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/.
It would be better to reformat the commit message a bit.
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/.
Add a blank line between paragraphs.
>
> Reported-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Joshua Perrett <jperrett256@xxxxxxxxx>
Do you have a Citrix email address?
> ---
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl_nic.c | 59 +++++++++--
> tools/libxl/md5.c | 266
> ++++++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/md5.h | 26 +++++
> 4 files changed, 346 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..37de5ffb04 100644
> --- a/tools/libxl/libxl_nic.c
> +++ b/tools/libxl/libxl_nic.c
> @@ -17,6 +17,16 @@
>
> #include "libxl_internal.h"
>
> +#include <string.h>
> +
> +#include "md5.h"
> +
> +#include <sys/types.h>
> +#include <ifaddrs.h>
> +#include <sys/socket.h>
> +#include <linux/if_packet.h>
> +#include <net/ethernet.h>
> +
This will break BSDs. They will probably need to be enclosed in ifdef
__linux__.
It isn't always clear at the beginning that Xen can use other Unix-like
systems as Dom0.
> int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> const char *mac, libxl_device_nic *nic)
> {
> @@ -53,8 +63,36 @@ int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> return rc;
> }
>
> +static int libxl__get_host_mac(unsigned char *buf)
> +{
> + int success = -1;
Change this to rc and use it to store libxl error code. Please read
CODING_STYLE in libxl.
> + struct ifaddrs *iface_list;
Blank line here.
> + if (getifaddrs(&iface_list) == 0) {
Is the list stable?
> + 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;
So the first mac is used?
> + if (s->sll_halen == 6) {
> + for (int i = 0; i < 6; i++) {
> + buf[i] = s->sll_addr[i];
> + }
This can probably be replaced by a call to memcpy.
> + if(buf[0] || buf[1] || buf[2] || buf[3] || buf[4] ||
> buf[5]) {
> + success = 0;
> + break;
> + }
> + }
> + }
> + }
> + freeifaddrs(iface_list);
> + } else {
> + perror("ERROR: getifaddrs\n");
Use one of the LOG macros here. You will need to pass in a gc to this
function.
> + }
Blank line here.
> + return success;
> +}
> +
> 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 +103,20 @@ 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];
> +
> + uint8_t hostmac[6] = {0};
> +
> + if(libxl__get_host_mac(hostmac)) {
> + perror("WARNING: failed to get host mac address\n");
LOG here. And you should probably bail instead of continuing?
Again, you might want to make hostmac a Linux only thing.
> + }
>
> - libxl_uuid_generate(&uuid);
> - r = libxl_uuid_bytearray(&uuid);
> + MD5_CTX ctx;
> + MD5Init(&ctx);
> + MD5Update(&ctx, hostmac, sizeof(hostmac));
> + MD5Update(&ctx, (uint8_t *) name, strlen(name));
> + MD5Update(&ctx, (uint8_t *) &nic_index, sizeof(nic_index));
> + MD5Final(r, &ctx);
>
> nic->mac[0] = 0x00;
> nic->mac[1] = 0x16;
> @@ -478,7 +525,7 @@ int libxl__device_nic_set_devids(libxl__gc *gc,
> libxl_domain_config *d_config,
> * but qemu needs the nic information to be complete.
> */
> ret = libxl__device_nic_setdefault(gc, domid, &d_config->nics[i],
> - false);
> + d_config->c_info.name, i, false);
> if (ret) {
> LOGD(ERROR, domid, "Unable to set nic defaults for nic %d", i);
> goto out;
> diff --git a/tools/libxl/md5.c b/tools/libxl/md5.c
> new file mode 100644
> index 0000000000..88ea13938a
> --- /dev/null
> +++ b/tools/libxl/md5.c
I have skipped this file.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |