[ovs-dev] [PATCH v4] acinclude: Autodetect DPDK location when configuring OVS

[Bodireddy, Bhanuprakash]

Thanks for the review Ben. I have sent out Patch V5 based on your comments.

- Bhanu Prakash.

> -----Original Message-----
> From: Ben Pfaff [mailto:blp at ovn.org]
> Sent: Tuesday, April 12, 2016 3:58 AM
> To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy at intel.com>
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v4] acinclude: Autodetect DPDK location when
> configuring OVS
> 
> On Thu, Mar 31, 2016 at 08:03:05PM +0100, Bhanuprakash Bodireddy wrote:
> > When using DPDK datapath, the OVS configure script requires the DPDK
> > build directory passed on --with-dpdk. This can be avoided if DPDK
> > library, headers are in standard compiler search paths.
> >
> > This patch fixes the problem by searching for DPDK libraries in
> > standard locations and configures OVS sources for dpdk datapath.
> >
> > If the install location is manually specified in "--with-dpdk"
> > autodiscovery shall be skipped.
> >
> > Signed-off-by: Bhanuprakash Bodireddy
> > <bhanuprakash.bodireddy at intel.com>
> 
> ...
> 
> > -  if test X"$with_dpdk" != X; then
> > -    RTE_SDK=$with_dpdk
> > +  AC_MSG_CHECKING([whether dpdk datapath is enabled])
> 
> == is not portable for "test", use = instead:
> > +  if test -z "$with_dpdk" || test "$with_dpdk" == no; then
> > +    AC_MSG_RESULT([no])
> > +    DPDKLIB_FOUND=false
> 
> Isn't the following "elif" test always true?  That is, can't it just be an "else"?
> 
> > +  elif test -n "$with_dpdk"; then
> > +    AC_MSG_RESULT([yes])
> > +    case "$with_dpdk" in
> > +      yes)
> > +        DPDK_AUTO_DISCOVER="true"
> > +        ;;
> > +      *)
> > +        DPDK_AUTO_DISCOVER="false"
> > +        ;;
> > +    esac
> >
> > -    DPDK_INCLUDE=$RTE_SDK/include
> > -    DPDK_LIB_DIR=$RTE_SDK/lib
> 
> Isn't the following "if" a little redundant given the previous "case"
> command, that is, why not integrate these into the cases?
> 
> > +    if $DPDK_AUTO_DISCOVER; then
> > +      DPDK_INCLUDE="/usr/local/include/dpdk -I/usr/include/dpdk"
> > +    else
> > +      DPDK_INCLUDE="$with_dpdk/include"
> > +      # If 'with_dpdk' is passed install directory, point to headers
> > +      # installed in $DESTDIR/$prefix/include/dpdk
> 
> This is really crunched together, why not add some whitespace to match the
> rest of the style:
> > +
> AC_CHECK_FILE([$DPDK_INCLUDE/rte_config.h],,[AC_CHECK_FILE([$DPDK_I
> NCLUDE/dpdk/rte_config.h],[DPDK_INCLUDE=$DPDK_INCLUDE/dpdk],[])])
> > +      DPDK_LIB_DIR="$with_dpdk/lib"
> > +    fi
> >      DPDK_LIB="-ldpdk"
> >      DPDK_EXTRA_LIB=""
> > -    RTE_SDK_FULL=`readlink -f $RTE_SDK`
> > +
> > +    ovs_save_CFLAGS="$CFLAGS"
> > +    ovs_save_LDFLAGS="$LDFLAGS"
> > +    CFLAGS="$CFLAGS -I$DPDK_INCLUDE"
> > +    if test "$DPDK_AUTO_DISCOVER" = "false"; then
> > +      LDFLAGS="$LDFLAGS -L${DPDK_LIB_DIR}"
> > +    fi
> >
> >      AC_COMPILE_IFELSE(
> > -      [AC_LANG_PROGRAM([#include
> <$RTE_SDK_FULL/include/rte_config.h>
> > +      [AC_LANG_PROGRAM([#include <rte_config.h>
> >  #if !RTE_LIBRTE_VHOST_USER
> >  #error
> >  #endif], [])],
> 
> The following line looks pretty randomly indented, I'd suggest that it should
> be two spaces farther in than the AC_LANG_PROGRAM keyword (and
> probably should start the [#include... on a separate line also indented the
> same amount):
> 
> >                      [], [AC_DEFINE([VHOST_CUSE], [1], [DPDK vhost-cuse support
> enabled, vhost-user disabled.])
> >                           DPDK_EXTRA_LIB="-lfuse"])
> >
> > -    ovs_save_CFLAGS="$CFLAGS"
> > -    ovs_save_LDFLAGS="$LDFLAGS"
> > -    LDFLAGS="$LDFLAGS -L$DPDK_LIB_DIR"
> > -    CFLAGS="$CFLAGS -I$DPDK_INCLUDE"
> > -
> >      # On some systems we have to add -ldl to link with dpdk
> >      #
> >      # This code, at first, tries to link without -ldl (""),
> 
> Thanks,
> 
> Ben.