On 03/30/2010 11:56 AM, Stefan Berger wrote:
Subject: Support for learning a VM's IP address
This patch implements support for learning a VM's IP address. It uses the pcap library to listen on the VM's backend network interface (tap) or the physical ethernet device (macvtap) and tries to capture packets with source or destination MAC address of the VM and learn from DHCP Offers, ARP traffic, or first-sent IPv4 packet what the IP address of the VM's interface is. This then allows to instantiate the network traffic filtering rules without the user having to provide the IP parameter somewhere in the filter description or in the interface description as a parameter. This only supports to detect the parameter IP, which is for the assumed single IPv4 address of a VM. There is not support for interfaces that may have multiple IP addresses (IP aliasing) or IPv6 that may then require more than one valid IP address to be detected. A VM can have multiple independent interfaces that each uses a different IP address and in that case it will be attempted to detect each one of the address independently.
Sounds interesting.
--- configure.ac | 39 ++
Should there also be an adjustment to the .rpm spec files, to mention that we now might depend on libpcap (and building of all features depends on libpcap-devel)?
Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c ... + +#ifdef HAVE_LIBPCAP +#include <pcap.h> +#endif
Fails 'make syntax-check' if you install cppi (which is now part of Fedora).
+ +#include <fcntl.h> +#include <sys/ioctl.h> + +#include <arpa/inet.h> +#include <net/ethernet.h> +#include <netinet/ip.h> +#include <netinet/udp.h> +#include <net/if_arp.h> +#include <linux/if.h>
Can we guarantee that this header always exists, or should it be conditionally included?
+ + +/* structure of an ARP request/reply message */ +struct f_arphdr { + struct arphdr arphdr; + uint8_t ar_sha[ETH_ALEN]; + uint32_t ar_sip; + uint8_t ar_tha[ETH_ALEN]; + uint32_t ar_tip; +} __attribute__((packed));
This assumes GCC; do we ever intend to support other compilers?
+/** + * virNWFilterLearnInit + * Initialization of this layer + */ +int +virNWFilterLearnInit(void) { + pendingLearnReq = virHashCreate(0); + if (!pendingLearnReq) { + virReportOOMError(); + return 1; + } + + if (virMutexInit(&pendingLearnReqLock)) { + virNWFilterLearnShutdown(); + return 1; + }
Is this function leaking memory if it fails mid-way through? Or are you requiring that the caller will call virNWFilterShutdown even if virNWFilterLearnInit returns failure?
Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -24,6 +24,9 @@ #include <config.h>
#include <stdint.h> +#include <sys/socket.h> +#include <sys/ioctl.h> +#include <linux/if.h>
Another instance of using a non-portable header. I've read through the patch, and didn't spot many glaring errors. I have not tested it, though, and would rather defer to others to also look through it before approving it, since it is rather large. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org