Eric Blake <eblake@xxxxxxxxxx> wrote on 03/31/2010 02:28:09 PM:
>
> 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)?
Correct.
>
> > 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).
I don't have that install. I know, indentation after the '#'.
>
> > +
> > +#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?
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?
>
> > +
> > +
> > +/* 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?
Hm, it should be #defined in internal.h, I guess that would solve part of the problem.
Are other compilers already being used ?
>
> > +/**
> > + * 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?
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.
>
> > 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.
Same issue, yes.
>
> 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.
Ok.
Stefan
>
> --
> Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
>
> [attachment "signature.asc" deleted by Stefan Berger/Watson/IBM]
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list