[libvirt] [PATCH v2] nwfilter: Support for learning a VM's IP address
Stefan Berger
stefanb at us.ibm.com
Wed Apr 7 23:19:58 UTC 2010
Daniel Veillard <veillard at redhat.com> wrote on 04/07/2010 05:22:22 PM:
> libvir-list, eblake, berrange
>
> Please respond to veillard
>
[...]
>
> In general the code looks nice to me, just a few things to point out
>
> > Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c
> [...]
> > +/* 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;
> > +
>
> the 3 structures requiring ATTRIBUTE_PACKED, actually f_arphdr might be
> the only one really requiring the packing, I don't know how the
> compilers would work withthe 2 * 8 uint8_t arrays of
> ether_vlan_header...
I think most of the compilers should define an equivalent for
__attribute__((packed) just to make structure defined for networking or
hardware easier accessible. #pragma pack(1) might be such a thing, which
then would have to be surrounded with
#if OTHER_COMPILER_THAN_GCC
# pragma pack(1)
#endif
...
#if OTHER_COMPILER_THAN_GCC
# pragma pop
#endif
> > +checkIf(const char *ifname, const unsigned char *macaddr)
> > +{
> [...]
> >
> > +int checkIf(const char *ifname, const unsigned char *macaddr);
> > +
> > #endif
>
> I think those 3 should really be cleaned up, either merged with
> another funcion as suggested, or at least be prefixed to avoid name
> pollution. Not extremely urgent especially if there is some merging
> with macvtap.c code needed.
Ok. The FIXME would remind me.
>
> > Index: libvirt-acl/src/libvirt_private.syms
> > ===================================================================
> > --- libvirt-acl.orig/src/libvirt_private.syms
> > +++ libvirt-acl/src/libvirt_private.syms
> > @@ -488,6 +488,8 @@ virNWFilterRegisterCallbackDriver;
> > virNWFilterTestUnassignDef;
> > virNWFilterConfLayerInit;
> > virNWFilterConfLayerShutdown;
> > +virNWFilterLockFilterUpdates;
> > +virNWFilterUnlockFilterUpdates;
> >
> >
> > #nwfilter_params.h
> > @@ -503,6 +505,16 @@ virNWFilterInstantiateFilter;
> > virNWFilterTeardownFilter;
> >
> >
> > +#nwfilter_learnipaddr.h
> > +ipAddressMap;
> > +ipAddressMapLock;
> > +pendingLearnReq;
> > +pendingLearnReqLock;
>
> as far as I can tell those 4 don't need to be exported as they are not
> used in any other module, and sine they don't have a virNWFilter prefix
> I think it's cleaner that way. They are actually static in your latest
> version.
Oh, I had some really interesting debugging hours behind me because there
were some weak symbols ('t') in libvirt and it was linking against another
part that was exporting the same symbols ('T'). So the code was there
twice if I remember this correctly - been a while. Sometimes static
variables were seen as being initialized, i.e., when access from code
within the nwfilter_learnip.c file and other times when access from
libvirt_conf.c they weren't. Also the static variables all of a sudden
were at two different locations, depending on what the path was that
invoked the code. Crazy. If I remember correctly, some of these entries
were the consequence of this... but now that I checked out the code from
git and ran a test with the 'clean-traffic' filter it may not be the case
- at least I did not see any strange crashes.
>
> > +virNWFilterGetIpAddrForIfname;
> > +virNWFilterDelIpAddrForIfname;
> > +virNWFilterLookupLearnReq;
> > +
> > +
> > # pci.h
> > pciGetDevice;
> > pciFreeDevice;
> [...]
>
> > Index: libvirt-acl/configure.ac
> > ===================================================================
> > --- libvirt-acl.orig/configure.ac
> > +++ libvirt-acl/configure.ac
> > @@ -41,6 +41,7 @@ XMLRPC_REQUIRED=1.14.0
> > HAL_REQUIRED=0.5.0
> > DEVMAPPER_REQUIRED=1.0.0
> > LIBCURL_REQUIRED="7.18.0"
> > +LIBPCAP_REQUIRED="1.0.0"
> >
> > dnl Checks for C compiler.
> > AC_PROG_CC
> > @@ -1045,6 +1046,39 @@ AC_SUBST([NUMACTL_CFLAGS])
> > AC_SUBST([NUMACTL_LIBS])
> >
> >
> > +dnl pcap lib
> > +LIBPCAP_CONFIG="pcap-config"
> > +LIBPCAP_CFLAGS=""
> > +LIBPCAP_LIBS=""
> > +LIBPCAP_FOUND="no"
> > +
> > +AC_ARG_WITH([libpcap], AC_HELP_STRING([--with-libpcap=@<:@PFX@:>@],
> > [libpcap location]))
> > +if test "$with_qemu" = "yes"; then
> > + if test "x$with_libpcap" != "xno" ; then
>
> Somehow we are binding this completely to the compilation of qemu,
> in the detection and in the spec file, it's fine for now but maybe
> should be relaxed at some point.
Currently it only works for Qemu...
Great that the code is 'in'.
Thanks.
Stefan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100407/a68cc662/attachment-0001.htm>
More information about the libvir-list
mailing list