[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