[libvirt] Adding and Clearing bridge addresses
I have been working on this patch to have libvirt optionally set static routes. So I found the function that adds both IPv4 and IPv6 addresses to the bridge in virnetdev.c. I found that besides the virNetDevAddIPv4Address() there is also virNetDevCleanIPv4Address(). I patterned my virNetDevAddGateway() after the virNetDevAddIPv4Address() function. What I am puzzled about is that it appears that nobody calls the Clear function. What don't I understand? It appears to me that the addresses and routes assigned to a bridge are all removed when the network is destroyed but otherwise remain. Gene
On 03/12/2013 01:45 PM, Gene Czarcinski wrote:
I have been working on this patch to have libvirt optionally set static routes.
So I found the function that adds both IPv4 and IPv6 addresses to the bridge in virnetdev.c. I found that besides the virNetDevAddIPv4Address() there is also virNetDevCleanIPv4Address(). I patterned my virNetDevAddGateway() after the virNetDevAddIPv4Address() function.
What I am puzzled about is that it appears that nobody calls the Clear function. What don't I understand?
Probably it's there just for completeness of API. The thing that I find strange is that these functions include "IPv4" in their names, in spite of working just as well for IPv6. It's very likely that if I dig back through the blame history (which won't be simple since the code has been moved into different files), I'll find that I originally wrote it (I have a vague memory of it), but don't think I would purposefully add IPv4 to the names... :-/
It appears to me that the addresses and routes assigned to a bridge are all removed when the network is destroyed but otherwise remain.
On Tue, Mar 12, 2013 at 03:11:56PM -0400, Laine Stump wrote:
On 03/12/2013 01:45 PM, Gene Czarcinski wrote:
I have been working on this patch to have libvirt optionally set static routes.
So I found the function that adds both IPv4 and IPv6 addresses to the bridge in virnetdev.c. I found that besides the virNetDevAddIPv4Address() there is also virNetDevCleanIPv4Address(). I patterned my virNetDevAddGateway() after the virNetDevAddIPv4Address() function.
What I am puzzled about is that it appears that nobody calls the Clear function. What don't I understand?
Probably it's there just for completeness of API.
The thing that I find strange is that these functions include "IPv4" in their names, in spite of working just as well for IPv6. It's very likely that if I dig back through the blame history (which won't be simple since the code has been moved into different files), I'll find that I originally wrote it (I have a vague memory of it), but don't think I would purposefully add IPv4 to the names... :-/
The virNetDevGetIPv4Address method uses an ioctl to get the address and this only works on IPv4 addrs. The intent was to turn the other two calls into ioctl() calls too, to kill off shell commands. Setting IPv6 addresses would require netlink usage instead IIRC 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 :|
On 03/12/2013 03:23 PM, Daniel P. Berrange wrote:
On Tue, Mar 12, 2013 at 03:11:56PM -0400, Laine Stump wrote:
On 03/12/2013 01:45 PM, Gene Czarcinski wrote:
I have been working on this patch to have libvirt optionally set static routes.
So I found the function that adds both IPv4 and IPv6 addresses to the bridge in virnetdev.c. I found that besides the virNetDevAddIPv4Address() there is also virNetDevCleanIPv4Address(). I patterned my virNetDevAddGateway() after the virNetDevAddIPv4Address() function.
What I am puzzled about is that it appears that nobody calls the Clear function. What don't I understand? Probably it's there just for completeness of API.
The thing that I find strange is that these functions include "IPv4" in their names, in spite of working just as well for IPv6. It's very likely that if I dig back through the blame history (which won't be simple since the code has been moved into different files), I'll find that I originally wrote it (I have a vague memory of it), but don't think I would purposefully add IPv4 to the names... :-/ The virNetDevGetIPv4Address method uses an ioctl to get the address and this only works on IPv4 addrs. The intent was to turn the other two calls into ioctl() calls too, to kill off shell commands.
Setting IPv6 addresses would require netlink usage instead IIRC
I seem to recall some comments in some source code I have browsed in the last couple days which stated that ioctl was NOT to be used because it could not be guaranteed long term ... I think it was in libvert code but it may have been in NetworkManager code. When you jump around to different projects, the details get confusing. Anyway, I like the way /usr/sbin/ip works because things just seem to work. A couple of things I have learned: 1. For IPv6 addr and route, the order does not seem to matter and the bridge can be offline. 2. Both IPv4 and IPv6 addr can be done on the offline bridge. 3. To set an IPv4 static route, the addresses need to be defined and the bridge online (IPv6 is not so picky). 4. You better have the unmasked portion of the address specified zero for the route ... that is, it better be a network address rather than a host address [I need to add some checks for this rather than letter the system issue a cryptic error message] 5. I have yet to figure out how to test for dhcp, etc. when via is defined so that I can issue an error message when it is created/defined [the xml is being parsed and read in]. 6. When the xml is being written for an IP definition, I currently error out. A better approach might be to issue an error message and suppress writing the xml. This needs further thought. 7. I have not come up with any tests. The only thing that might be meaningful is if a bad network definition was created so that it would get errors. I am not sure how to do this with the tests. There might be something in the networkxml2xmltest. I should have something with works real soon and will send the patch up for review. Gene
On 03/13/2013 08:34 AM, Gene Czarcinski wrote:
On 03/12/2013 03:23 PM, Daniel P. Berrange wrote:
On Tue, Mar 12, 2013 at 03:11:56PM -0400, Laine Stump wrote:
On 03/12/2013 01:45 PM, Gene Czarcinski wrote:
I have been working on this patch to have libvirt optionally set static routes.
So I found the function that adds both IPv4 and IPv6 addresses to the bridge in virnetdev.c. I found that besides the virNetDevAddIPv4Address() there is also virNetDevCleanIPv4Address(). I patterned my virNetDevAddGateway() after the virNetDevAddIPv4Address() function.
What I am puzzled about is that it appears that nobody calls the Clear function. What don't I understand? Probably it's there just for completeness of API.
The thing that I find strange is that these functions include "IPv4" in their names, in spite of working just as well for IPv6. It's very likely that if I dig back through the blame history (which won't be simple since the code has been moved into different files), I'll find that I originally wrote it (I have a vague memory of it), but don't think I would purposefully add IPv4 to the names... :-/ The virNetDevGetIPv4Address method uses an ioctl to get the address and this only works on IPv4 addrs. The intent was to turn the other two calls into ioctl() calls too, to kill off shell commands.
Setting IPv6 addresses would require netlink usage instead IIRC
I seem to recall some comments in some source code I have browsed in the last couple days which stated that ioctl was NOT to be used because it could not be guaranteed long term ...
I hadn't heard anything about that, but it is true that the ioctls can't be used for IPv6 addresses, or for multiple IP addresses (v4 or v6) on an interface. That's why what we really need to do is to switch all these functions to use netlink (I guess with /usr/bin/ip as a fallback).
I think it was in libvert code but it may have been in NetworkManager code. When you jump around to different projects, the details get confusing.
Anyway, I like the way /usr/sbin/ip works because things just seem to work.
There are two problems with using /usr/bin/ip: 1) it makes libvirt dependent on the iproute2 package 2) it adds one more external binary that libvirt must run as root, creating a potential for exploit. The problem with using ioctl(SIOCSIFADDR) is that it only supports a single IP address (IPv4 only) per interface. The "good" way to set IP addresses is (as Daniel mentioned) netlink messages, at least on those platforms that have netlink. /usr/bin/ip itself uses netlink. When I added basic support for IPv6 to libvirt networks, libvirt didn't yet have macvtap support (which also requires netlink), so none of the infrastructure to do netlink was there yet, and adding a dependency on iproute2 seemed like a reasonable expedient way to get the functionality we needed. Three years later, we have other parts of the code using netlink, and it would be nice to remove the dependency on iproute2 and the exec of an external binary.
A couple of things I have learned:
1. For IPv6 addr and route, the order does not seem to matter and the bridge can be offline.
2. Both IPv4 and IPv6 addr can be done on the offline bridge.
3. To set an IPv4 static route, the addresses need to be defined and the bridge online (IPv6 is not so picky).
4. You better have the unmasked portion of the address specified zero for the route ... that is, it better be a network address rather than a host address [I need to add some checks for this rather than letter the system issue a cryptic error message]
5. I have yet to figure out how to test for dhcp, etc. when via is defined so that I can issue an error message when it is created/defined [the xml is being parsed and read in].
6. When the xml is being written for an IP definition, I currently error out. A better approach might be to issue an error message and suppress writing the xml. This needs further thought.
7. I have not come up with any tests. The only thing that might be meaningful is if a bad network definition was created so that it would get errors. I am not sure how to do this with the tests. There might be something in the networkxml2xmltest.
I should have something with works real soon and will send the patch up for review.
Gene
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Daniel P. Berrange -
Gene Czarcinski -
Laine Stump