6:19 p.m.
Daniel Veillard <veillard(a)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