<tt>Eric Blake <eblake@redhat.com> wrote on 03/31/2010 02:28:09 PM: </tt> <tt>> > 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)?</tt> <tt>Correct.</tt> <tt> > > > 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).</tt> <tt>I don't have that install. I know, indentation after the '#'.</tt> <tt> > > > + > > +#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?</tt> <tt>I guess this would only work on Linux, not on OpenSolaris or mingw. So, yes, conditionally including is probably the right thing. What would be the right condition to use?</tt> <tt> > > > + > > + > > +/* 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?</tt> <tt>Hm, it should be #defined in internal.h, I guess that would solve part of the problem.</tt> <tt>Are other compilers already being used ?</tt> <tt> > > > +/** > > + * 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?</tt> <tt>No, it shouldn't. Rather than cleaning up the objects that were created in this function before it failed I call into the shutdown function to free them.</tt> <tt> > > > 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.</tt> <tt>Same issue, yes.</tt> <tt> > > 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.</tt> <tt>Ok.</tt> <tt> Stefan</tt> <tt> > > -- > Eric Blake eblake@redhat.com +1-801-349-2682 > Libvirt virtualization library </tt><a href=http://libvirt.org/><tt>http://libvirt.org</tt></a><tt> > > [attachment "signature.asc" deleted by Stefan Berger/Watson/IBM] </tt>