[libvirt] [PATCHv2] virNetDevVethCreate: assign container if name based on parent if name
Replace the loop trying to find a free veth interface name for the container
by assigning the container if name to parent name + 'p' by default.
Interface name selection logic is susceptible to race conditions, so try to
select just one name by default and use that as a template for the second
name. The parent name can also be overriden in domain configuration.
Signed-off-by: Oskari Saarenmaa o...@ohmu.fi
---
v2: generate first name as before (if it wasn't given by the caller), and
use the parent if name as a template for the container if name so that the
caller doesn't have to select two names (which is not possible at the
moment.)
src/util/virnetdevveth.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index 039767f..91e2829 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -114,20 +114,14 @@ int virNetDevVethCreate(char** veth1, char** veth2)
}
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;
-}
-
VIR_DEBUG(Assigned guest: %s, *veth2);
veth2_alloc = true;
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virNetDevVethCreate: assign container if name based on parent if name
On Thu, Aug 29, 2013 at 01:00:15PM +0300, Oskari Saarenmaa wrote: Replace the loop trying to find a free veth interface name for the container by assigning the container if name to parent name + 'p' by default. Interface name selection logic is susceptible to race conditions, so try to select just one name by default and use that as a template for the second name. The parent name can also be overriden in domain configuration. This doesn't do anything to solve the race condition AFAICT. You still have the window between finding a free name, and calling the ip command to allocate it. What we need here is a change that will catch the failure from the ip command, and then go back to the start and find free names and retry the ip command. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virNetDevVethCreate: assign container if name based on parent if name
On Thu, Aug 29, 2013 at 11:28:43AM +0100, Daniel P. Berrange wrote: On Thu, Aug 29, 2013 at 01:00:15PM +0300, Oskari Saarenmaa wrote: Replace the loop trying to find a free veth interface name for the container by assigning the container if name to parent name + 'p' by default. Interface name selection logic is susceptible to race conditions, so try to select just one name by default and use that as a template for the second name. The parent name can also be overriden in domain configuration. This doesn't do anything to solve the race condition AFAICT. You still have the window between finding a free name, and calling the ip command to allocate it. That's true, but I think this change makes sense as a standalone change for now; it makes sure libvirt uses the caller assigned names (if any) which let's the caller work around this by selecting the name they like. What we need here is a change that will catch the failure from the ip command, and then go back to the start and find free names and retry the ip command. I don't like this approach. It would require us to parse ip's stderr which would be quite a bit of fragile code to handle a case that shouldn't ever happen. We should just select a unique interface name in the first place, I don't see any benefits in trying to force interface names to use low integers in their names. / Oskari -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virNetDevVethCreate: assign container if name based on parent if name
On Thu, Aug 29, 2013 at 02:46:43PM +0300, Oskari Saarenmaa wrote: On Thu, Aug 29, 2013 at 11:28:43AM +0100, Daniel P. Berrange wrote: On Thu, Aug 29, 2013 at 01:00:15PM +0300, Oskari Saarenmaa wrote: Replace the loop trying to find a free veth interface name for the container by assigning the container if name to parent name + 'p' by default. Interface name selection logic is susceptible to race conditions, so try to select just one name by default and use that as a template for the second name. The parent name can also be overriden in domain configuration. This doesn't do anything to solve the race condition AFAICT. You still have the window between finding a free name, and calling the ip command to allocate it. That's true, but I think this change makes sense as a standalone change for now; it makes sure libvirt uses the caller assigned names (if any) which let's the caller work around this by selecting the name they like. What we need here is a change that will catch the failure from the ip command, and then go back to the start and find free names and retry the ip command. I don't like this approach. It would require us to parse ip's stderr which would be quite a bit of fragile code to handle a case that shouldn't ever happen. We should just select a unique interface name in the first place, I don't see any benefits in trying to force interface names to use low integers in their names. That is not fixing the race condition at all. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
