Thanks for the review Ben. I have sent out Patch V5 based on your comments.
- Bhanu Prakash. > -----Original Message----- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Tuesday, April 12, 2016 3:58 AM > To: Bodireddy, Bhanuprakash <bhanuprakash.bodire...@intel.com> > Cc: dev@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.bodire...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
