Re: [libvirt] [PATCH] nwfilter: Support for learning a VM's IP address

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

> 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

> + > +#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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org [attachment "signature.asc" deleted by Stefan Berger/Watson/IBM]