Daniel Veillard <veillard@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