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

[Shriram Rajagopalan <rshriram@cs.ubc.ca>]

[-- Attachment #1.1: Type: text/plain, Size: 3602 bytes --]

On Thu, Jun 5, 2014 at 8:48 PM, Hongyang Yang <yanghy@cn.fujitsu.com> 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@cs.ubc.ca>
>>>
>>
>> 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@cn.fujitsu.com>
>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>> Reviewed-by: Wen Congyang <wency@cn.fujitsu.com>
>>>
>> ...
>>
>>> +# 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.
>
>

[-- Attachment #1.2: Type: text/html, Size: 6138 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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