Re: [Xen-devel] [PATCH v10 2/5] remus: add libnl3 dependency for network buffering support

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On Thu, Jun 5, 2014 at 8:48 PM, Hongyang Yang <yanghy@xxxxxxxxxxxxxx> wrote:

On 06/06/2014 12:18 AM, Ian Jackson wrote:

Yang Hongyang writes ("[PATCH v10 2/5] remus: add libnl3 dependency for network buffering support"):

Libnl3 is required for controlling Remus network buffering.
This patch adds dependency on libnl3 (>= 3.2.8) to autoconf scripts.
Also provide ability to configure tools without libnl3 support, that
is without network buffering support.

This patch looks broadly good to me. ÂI have some very minor comments
about the details.

when there's no network buffering support,libxl__netbuffer_enabled()
returns 0, otherwise returns 1.

The commit message should explicitly state that callers will be
introduced in the rest of the series.

Signed-off-by: Shriram Rajagopalan <rshriram@xxxxxxxxx>

For a patch which changes configure.ac, it would be helpful to add a
reminder (for the commiter) to rerun autogen.sh. ÂThis should ideally
appear just before the first Signed-off-by. ÂThe committer should
delete the note, and rerun autogen.sh, as they apply the patch.

Thanks, will add the comment.

Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
Reviewed-by: Wen Congyang <wency@xxxxxxxxxxxxxx>

...

+# Check for libnl3 >=3.2.8. If present enable remus network buffering.
+PKG_CHECK_MODULES(LIBNL3, [libnl-3.0 >= 3.2.8 libnl-route-3.0 >= 3.2.8],
+ Â Â[libnl3_lib="y"], [libnl3_lib="n"])
+
+AS_IF([test "x$libnl3_lib" = "xn" ], [
+ Â ÂAC_MSG_WARN([Disabling support for Remus network buffering.
+ Â ÂPlease install libnl3 libraries, command line tools and devel
+ Â Âheaders - version 3.2.8 or higher])
+ Â ÂAC_SUBST(remus_netbuf, [n])
+ Â Â],[
+ Â ÂAC_SUBST(LIBNL3_LIBS)
+ Â ÂAC_SUBST(LIBNL3_CFLAGS)

It might be better to put these AC_SUBSTs into the main body of
configure.ac ? ÂLike this:

  diff --git a/tools/configure.ac b/tools/configure.ac
  index 38d2d05..ee36707 100644
  --- a/tools/configure.ac
  +++ b/tools/configure.ac
  @@ -257,10 +257,11 @@ AS_IF([test "x$libnl3_lib" = "xn" ], [
    Âheaders - version 3.2.8 or higher])
    ÂAC_SUBST(remus_netbuf, [n])
    Â],[
  -  ÂAC_SUBST(LIBNL3_LIBS)
  -  ÂAC_SUBST(LIBNL3_CFLAGS)
    ÂAC_SUBST(remus_netbuf, [y])
  Â])

  +AC_SUBST(LIBNL3_LIBS)
  +AC_SUBST(LIBNL3_CFLAGS)
  +
  ÂAC_OUTPUT()

yes, i'll try that, if we do these, then the following check of CONFIG_REMUS_NETBUF in libxl Makefile will no longer need:

ÂLIBXL_LIBS =
ÂLIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
+ifeq ($(CONFIG_REMUS_NETBUF),y)
+LIBXL_LIBS += $(LIBNL3_LIBS)
+endif

ÂCFLAGS_LIBXL += $(CFLAGS_libxenctrl)
ÂCFLAGS_LIBXL += $(CFLAGS_libxenguest)
ÂCFLAGS_LIBXL += $(CFLAGS_libxenstore)
ÂCFLAGS_LIBXL += $(CFLAGS_libblktapctl)
+ifeq ($(CONFIG_REMUS_NETBUF),y)
+CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
+endif
ÂCFLAGS_LIBXL += -Wshadow

IIRC, starting with GCC 4.6, that check is not needed anyway, because

gcc will prevent unused shared libraries from being linked into libxl. However,

with earlier GCC versions, these libraries will get linked unless the -as-needed flag

is supplied. Which was the reason I had that check in place.

Â

Thanks,
Ian.
.

--
Thanks,
Yang.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel