[libvirt] [PATCH] virNetDevVethCreate: assign names based on mac address by default
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name. Signed-off-by: Oskari Saarenmaa <os@ohmu.fi> --- src/lxc/lxc_process.c | 2 +- src/util/virnetdevveth.c | 81 +++++++++--------------------------------------- src/util/virnetdevveth.h | 6 ++-- 3 files changed, 19 insertions(+), 70 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 4835bd5..956bbdb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -243,7 +243,7 @@ char *virLXCProcessSetupInterfaceBridged(virConnectPtr conn, VIR_DEBUG("calling vethCreate()"); parentVeth = net->ifname; - if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0) + if (virNetDevVethCreate(&parentVeth, &containerVeth, &net->mac) < 0) goto cleanup; VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth); diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index 039767f..c0e3e93 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -38,96 +38,46 @@ /* Functions */ /** - * virNetDevVethGetFreeName: - * @veth: pointer to store returned name for veth device - * @startDev: device number to start at (x in vethx) - * - * Looks in /sys/class/net/ to find the first available veth device - * name. - * - * Returns non-negative device number on success or -1 in case of error - */ -static int virNetDevVethGetFreeName(char **veth, int startDev) -{ - int devNum = startDev-1; - char *path = NULL; - - VIR_DEBUG("Find free from veth%d", startDev); - do { - VIR_FREE(path); - ++devNum; - if (virAsprintf(&path, "/sys/class/net/veth%d/", devNum) < 0) - return -1; - VIR_DEBUG("Probe %s", path); - } while (virFileExists(path)); - VIR_FREE(path); - - if (virAsprintf(veth, "veth%d", devNum) < 0) - return -1; - - return devNum; -} - -/** * virNetDevVethCreate: * @veth1: pointer to name for parent end of veth pair * @veth2: pointer to return name for container end of veth pair + * @mac: mac address of the device * * Creates a veth device pair using the ip command: * ip link add veth1 type veth peer name veth2 - * If veth1 points to NULL on entry, it will be a valid interface on - * return. veth2 should point to NULL on entry. * - * NOTE: If veth1 and veth2 names are not specified, ip will auto assign - * names. There seems to be two problems here - - * 1) There doesn't seem to be a way to determine the names of the - * devices that it creates. They show up in ip link show and - * under /sys/class/net/ however there is no guarantee that they - * are the devices that this process just created. - * 2) Once one of the veth devices is moved to another namespace, it - * is no longer visible in the parent namespace. This seems to - * confuse the name assignment causing it to fail with File exists. - * Because of these issues, this function currently allocates names - * prior to using the ip command, and returns any allocated names - * to the caller. + * If veth1 or veth2 points to NULL on entry, they will be a valid interface + * on return. The name is generated based on the mac address given. * * Returns 0 on success or -1 in case of error */ -int virNetDevVethCreate(char** veth1, char** veth2) +int virNetDevVethCreate(char** veth1, char** veth2, const virMacAddrPtr mac) { - int rc = -1; const char *argv[] = { "ip", "link", "add", NULL, "type", "veth", "peer", "name", NULL, NULL }; - int vethDev = 0; bool veth1_alloc = false; bool veth2_alloc = false; VIR_DEBUG("Host: %s guest: %s", NULLSTR(*veth1), NULLSTR(*veth2)); if (*veth1 == NULL) { - if ((vethDev = virNetDevVethGetFreeName(veth1, vethDev)) < 0) - goto cleanup; + /* Use the last three bytes of the mac address as our if name */ + if (virAsprintf(veth1, "veth%02x%02x%02x", + mac->addr[3], mac->addr[4], mac->addr[5]) < 0) + return -1; VIR_DEBUG("Assigned host: %s", *veth1); veth1_alloc = true; - vethDev++; } argv[3] = *veth1; - while (*veth2 == NULL) { - if ((vethDev = virNetDevVethGetFreeName(veth2, vethDev)) < 0) { + if (*veth2 == NULL) { + /* Append a 'p' to veth1 if name */ + if (virAsprintf(veth2, "%sp", *veth1) < 0) { if (veth1_alloc) VIR_FREE(*veth1); - goto cleanup; - } - - /* Just make sure they didn't accidentally get same name */ - if (STREQ(*veth1, *veth2)) { - vethDev++; - VIR_FREE(*veth2); - continue; + return -1; } - VIR_DEBUG("Assigned guest: %s", *veth2); veth2_alloc = true; } @@ -139,13 +89,10 @@ int virNetDevVethCreate(char** veth1, char** veth2) VIR_FREE(*veth1); if (veth2_alloc) VIR_FREE(*veth2); - goto cleanup; + return -1; } - rc = 0; - -cleanup: - return rc; + return 0; } /** diff --git a/src/util/virnetdevveth.h b/src/util/virnetdevveth.h index 4c220c1..175fbfa 100644 --- a/src/util/virnetdevveth.h +++ b/src/util/virnetdevveth.h @@ -25,10 +25,12 @@ # define __VIR_NETDEV_VETH_H__ # include "internal.h" +# include "virmacaddr.h" /* Function declarations */ -int virNetDevVethCreate(char **veth1, char **veth2) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevVethCreate(char **veth1, char **veth2, const virMacAddrPtr mac) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; int virNetDevVethDelete(const char *veth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- 1.8.3.1
On 28.08.2013 23:05, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name.
Signed-off-by: Oskari Saarenmaa <os@ohmu.fi>
--- src/lxc/lxc_process.c | 2 +- src/util/virnetdevveth.c | 81 +++++++++--------------------------------------- src/util/virnetdevveth.h | 6 ++-- 3 files changed, 19 insertions(+), 70 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 4835bd5..956bbdb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -243,7 +243,7 @@ char *virLXCProcessSetupInterfaceBridged(virConnectPtr conn,
VIR_DEBUG("calling vethCreate()"); parentVeth = net->ifname; - if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0) + if (virNetDevVethCreate(&parentVeth, &containerVeth, &net->mac) < 0) goto cleanup; VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index 039767f..c0e3e93 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -38,96 +38,46 @@
/* Functions */ /** - * virNetDevVethGetFreeName: - * @veth: pointer to store returned name for veth device - * @startDev: device number to start at (x in vethx) - * - * Looks in /sys/class/net/ to find the first available veth device - * name. - * - * Returns non-negative device number on success or -1 in case of error - */ -static int virNetDevVethGetFreeName(char **veth, int startDev) -{ - int devNum = startDev-1; - char *path = NULL; - - VIR_DEBUG("Find free from veth%d", startDev); - do { - VIR_FREE(path); - ++devNum; - if (virAsprintf(&path, "/sys/class/net/veth%d/", devNum) < 0) - return -1; - VIR_DEBUG("Probe %s", path); - } while (virFileExists(path)); - VIR_FREE(path); - - if (virAsprintf(veth, "veth%d", devNum) < 0) - return -1; - - return devNum; -} - -/** * virNetDevVethCreate: * @veth1: pointer to name for parent end of veth pair * @veth2: pointer to return name for container end of veth pair + * @mac: mac address of the device * * Creates a veth device pair using the ip command: * ip link add veth1 type veth peer name veth2 - * If veth1 points to NULL on entry, it will be a valid interface on - * return. veth2 should point to NULL on entry. * - * NOTE: If veth1 and veth2 names are not specified, ip will auto assign - * names. There seems to be two problems here - - * 1) There doesn't seem to be a way to determine the names of the - * devices that it creates. They show up in ip link show and - * under /sys/class/net/ however there is no guarantee that they - * are the devices that this process just created. - * 2) Once one of the veth devices is moved to another namespace, it - * is no longer visible in the parent namespace. This seems to - * confuse the name assignment causing it to fail with File exists. - * Because of these issues, this function currently allocates names - * prior to using the ip command, and returns any allocated names - * to the caller. + * If veth1 or veth2 points to NULL on entry, they will be a valid interface + * on return. The name is generated based on the mac address given. * * Returns 0 on success or -1 in case of error */ -int virNetDevVethCreate(char** veth1, char** veth2) +int virNetDevVethCreate(char** veth1, char** veth2, const virMacAddrPtr mac) { - int rc = -1; const char *argv[] = { "ip", "link", "add", NULL, "type", "veth", "peer", "name", NULL, NULL }; - int vethDev = 0; bool veth1_alloc = false; bool veth2_alloc = false;
VIR_DEBUG("Host: %s guest: %s", NULLSTR(*veth1), NULLSTR(*veth2));
if (*veth1 == NULL) { - if ((vethDev = virNetDevVethGetFreeName(veth1, vethDev)) < 0) - goto cleanup; + /* Use the last three bytes of the mac address as our if name */ + if (virAsprintf(veth1, "veth%02x%02x%02x", + mac->addr[3], mac->addr[4], mac->addr[5]) < 0)
I don't think it is a good idea. What it user defines MAC addresses in a way that the last three bytes are the same? E.g. 00:11:22:33:44:55 00:22:11:33:44:55 (there are plenty of possibilities). With your code we would create veth334455 for the first domain and there's nothing left for the other one but eyes to cry. Moreover, there's no race now, as we use global lock to mutually exclude call to virNetDevVethCreate. Although, I must admit it's only within a single driver, I think. So if there are two domains starting (one in qemu driver the other one in lxc, for instance) there can be race. But this should be solved in a different way then. The virNetDevVethCreate should be turned into virObjectLockable and a veth should be allocated by calling a method. Michal
On Thu, Aug 29, 2013 at 10:10:36AM +0200, Michal Privoznik wrote:
On 28.08.2013 23:05, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name. I don't think it is a good idea. What it user defines MAC addresses in a way that the last three bytes are the same? E.g.
00:11:22:33:44:55 00:22:11:33:44:55
(there are plenty of possibilities). With your code we would create veth334455 for the first domain and there's nothing left for the other one but eyes to cry.
Sure, it's possible for the user to assign addresses like this, but I believe most mac addresses are assigned randomly, that's what libvirt does by default, no? Also, it's possible for the user to override the interface name if they want to. Currently it's only possible to set the first interface name, but there's no way to set the name of the second interface which causes failures because of races. In any case it makes sense to set the second interface's name based on the first interface's name so we don't have to try to come up with two different unique names.
Moreover, there's no race now, as we use global lock to mutually exclude call to virNetDevVethCreate. Although, I must admit it's only within a single driver, I think. So if there are two domains starting (one in qemu driver the other one in lxc, for instance) there can be race. But this should be solved in a different way then. The virNetDevVethCreate should be turned into virObjectLockable and a veth should be allocated by calling a method.
I ran into this issue repeatedly when I tried to start multiple LXC domains simultaneously from my application (https://github.com/melor/poni) which runs libvirt operations in their own threads. / Oskari
On 08/29/2013 04:20 PM, Oskari Saarenmaa wrote:
On Thu, Aug 29, 2013 at 10:10:36AM +0200, Michal Privoznik wrote:
On 28.08.2013 23:05, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name. I don't think it is a good idea. What it user defines MAC addresses in a way that the last three bytes are the same? E.g.
00:11:22:33:44:55 00:22:11:33:44:55
(there are plenty of possibilities). With your code we would create veth334455 for the first domain and there's nothing left for the other one but eyes to cry.
Sure, it's possible for the user to assign addresses like this, but I believe most mac addresses are assigned randomly, that's what libvirt does by default, no? Also, it's possible for the user to override the interface name if they want to. Currently it's only possible to set the first interface name, but there's no way to set the name of the second interface which causes failures because of races. In any case it makes sense to set the second interface's name based on the first interface's name so we don't have to try to come up with two different unique names.
Moreover, there's no race now, as we use global lock to mutually exclude call to virNetDevVethCreate. Although, I must admit it's only within a single driver, I think. So if there are two domains starting (one in qemu driver the other one in lxc, for instance) there can be race. But this should be solved in a different way then. The virNetDevVethCreate should be turned into virObjectLockable and a veth should be allocated by calling a method.
I ran into this issue repeatedly when I tried to start multiple LXC domains simultaneously from my application (https://github.com/melor/poni) which runs libvirt operations in their own threads.
It's easy to be resolved. give the parentVeth and containerVeth a name by default.
29.08.2013 11:36, Gao feng kirjoitti:
On 08/29/2013 04:20 PM, Oskari Saarenmaa wrote:
On Thu, Aug 29, 2013 at 10:10:36AM +0200, Michal Privoznik wrote:
On 28.08.2013 23:05, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name. I don't think it is a good idea. What it user defines MAC addresses in a way that the last three bytes are the same? E.g.
00:11:22:33:44:55 00:22:11:33:44:55
(there are plenty of possibilities). With your code we would create veth334455 for the first domain and there's nothing left for the other one but eyes to cry.
Sure, it's possible for the user to assign addresses like this, but I believe most mac addresses are assigned randomly, that's what libvirt does by default, no? Also, it's possible for the user to override the interface name if they want to. Currently it's only possible to set the first interface name, but there's no way to set the name of the second interface which causes failures because of races. In any case it makes sense to set the second interface's name based on the first interface's name so we don't have to try to come up with two different unique names.
Moreover, there's no race now, as we use global lock to mutually exclude call to virNetDevVethCreate. Although, I must admit it's only within a single driver, I think. So if there are two domains starting (one in qemu driver the other one in lxc, for instance) there can be race. But this should be solved in a different way then. The virNetDevVethCreate should be turned into virObjectLockable and a veth should be allocated by calling a method.
I ran into this issue repeatedly when I tried to start multiple LXC domains simultaneously from my application (https://github.com/melor/poni) which runs libvirt operations in their own threads.
It's easy to be resolved. give the parentVeth and containerVeth a name by default.
At the moment virLXCProcessSetupInterfaceBridged always passes a NULL for the container interface name and while it would be possible to make it configurable like the parent interface name, I believe it makes more sense to just do the right thing by default. The suggested patch also greatly simplifies name selection by removing the loops trying to find a supposedly unused interface name. If you don't like using mac address in the interface name we could just replace it with a random string with a loop checking that the selected name wasn't in use when it was selected. The current approach of trying to use a name with a low number just doesn't work in an environment where another process does the same thing. / Oskari
On 08/29/2013 04:52 PM, Oskari Saarenmaa wrote:
29.08.2013 11:36, Gao feng kirjoitti:
On 08/29/2013 04:20 PM, Oskari Saarenmaa wrote:
On Thu, Aug 29, 2013 at 10:10:36AM +0200, Michal Privoznik wrote:
On 28.08.2013 23:05, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name. I don't think it is a good idea. What it user defines MAC addresses in a way that the last three bytes are the same? E.g.
00:11:22:33:44:55 00:22:11:33:44:55
(there are plenty of possibilities). With your code we would create veth334455 for the first domain and there's nothing left for the other one but eyes to cry.
Sure, it's possible for the user to assign addresses like this, but I believe most mac addresses are assigned randomly, that's what libvirt does by default, no? Also, it's possible for the user to override the interface name if they want to. Currently it's only possible to set the first interface name, but there's no way to set the name of the second interface which causes failures because of races. In any case it makes sense to set the second interface's name based on the first interface's name so we don't have to try to come up with two different unique names.
Moreover, there's no race now, as we use global lock to mutually exclude call to virNetDevVethCreate. Although, I must admit it's only within a single driver, I think. So if there are two domains starting (one in qemu driver the other one in lxc, for instance) there can be race. But this should be solved in a different way then. The virNetDevVethCreate should be turned into virObjectLockable and a veth should be allocated by calling a method.
I ran into this issue repeatedly when I tried to start multiple LXC domains simultaneously from my application (https://github.com/melor/poni) which runs libvirt operations in their own threads.
It's easy to be resolved. give the parentVeth and containerVeth a name by default.
At the moment virLXCProcessSetupInterfaceBridged always passes a NULL for the container interface name and while it would be possible to make it configurable like the parent interface name, I believe it makes more sense to just do the right thing by default.
Agree.
The suggested patch also greatly simplifies name selection by removing the loops trying to find a supposedly unused interface name. If you don't like using mac address in the interface name we could just replace it with a random string with a loop checking that the selected name wasn't in use when it was selected. The current approach of trying to use a name with a low number just doesn't work in an environment where another process does the same thing.
Use mac address to create an unique name is not a good idea, I suggest you pass an index to virLXCProcessSetupInterfaceBridged. use this index and domain_name to create an unique name. Such as, for the first net interface for domain "fedora19" parentVeth's name is "fedora19-host-1", containerVeth is "fedora19-container-1" for the second interface, parentVeth's name is "fedora19-host-2", containerVeth is "fedora19-container-2" ... find out a proper name for these veth device :)
29.08.2013 12:11, Gao feng kirjoitti:
The suggested patch also greatly simplifies name selection by removing the loops trying to find a supposedly unused interface name. If you don't like using mac address in the interface name we could just replace it with a random string with a loop checking that the selected name wasn't in use when it was selected. The current approach of trying to use a name with a low number just doesn't work in an environment where another process does the same thing.
Use mac address to create an unique name is not a good idea, I suggest you pass an index to virLXCProcessSetupInterfaceBridged. use this index and domain_name to create an unique name.
Such as, for the first net interface for domain "fedora19" parentVeth's name is "fedora19-host-1", containerVeth is "fedora19-container-1" for the second interface, parentVeth's name is "fedora19-host-2", containerVeth is "fedora19-container-2" ...
find out a proper name for these veth device :)
Interface names are limited to 16 characters, so we can't really use the full domain name there and I suppose it makes sense to prefix them with "veth". That leaves 12 characters for the which is enough for a random name that will effectively never collide with anything else, but I'd still prefer naming them according to the mac address. The randomly generated address with 3 random bytes is unique enough to be used as a mac address in a network - it should be unique enough to be used as an interface name on a single host (and you can still override it in domain configuration if you want to.) / Oskari
On Thu, Aug 29, 2013 at 12:05:49AM +0300, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name.
Signed-off-by: Oskari Saarenmaa <os@ohmu.fi>
NACK, I really don't like this proposed naming scheme. If there is a rac with our current code we should be fixing that race. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Gao feng -
Michal Privoznik -
Oskari Saarenmaa