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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 323 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100331/b410ee12/attachment-0001.sig>