[SSSD] [PATCH] Provide libnl3 support

Jakub Hrozek jhrozek at redhat.com
Thu Mar 21 16:21:09 UTC 2013


On Thu, Mar 21, 2013 at 03:25:16PM +0100, Ondrej Kos wrote:
> On 03/07/2013 08:39 PM, Jakub Hrozek wrote:
> >On Thu, Mar 07, 2013 at 02:17:19PM +0100, Ondrej Kos wrote:
> >>Hi,
> >>
> >>Attached find patch introducing support of the libnl3 netlink
> >>library. The SSSD now builds with libnl3 by default, but is still
> >>configurable to use the libnl in version 1 by adding configure
> >>parameter
> >>--with-netlink-lib=libnl
> >>
> >
> >The attached patch "only" converts to being able to use the new API.
> >There is also one more enhancement I wanted to do when we move to libnl3
> >which is being able to detect when IPv6 address lifetime has been bumped
> >but the address has not changed really.
> >
> >If you run sssd in any environment that also has IPv6 addresses with
> >libnl1 you would receive RTM_NEWADDR notification even when nothing has
> >seemingly changed. Can you check if it's still the case with libnl3
> >and if it is, then also check if there is any way to filter these out?
> >
> 
> This is still happening and I couldn't find any workaround.tgr isn't
> responding now, so I'd put it in another patch, when I'll have some
> pointers from him.
> 

OK, but please open a ticket.

> >That would be separate patch (and even a ticket if you like). I vaguely
> >remember that tgr told me this was possible with libnl3 but I can't find
> >the details right now.
> >
> >
> >I'm thinking we might want to keep the same options (so that 3rd party
> >build that use --without-libnl would continue working). Defaulting to
> >another version of library would be OK in a new upstream release (if
> >called out in the release notes). If libnl3 is not available but libnl1
> >is, we should just fall back to libnl1 I think.
> >
> >So maybe we should keep the --with-libnl switch that could have these
> >values:
> >     * auto (default) - try libnl3, then fall back to libnl1, if neither
> >       is found, warn
> >     * libnl1 - explicitly request libnl1, if not available, fail
> >     * libnl3 - explicitly request libnl3, if not available, fail
> >     * --without-libnl - disable libnl support explicitly, no checks
> >
> 
> The options are now as you proposed:
> 
> - nothing specified:
> 	tries the libnl3
> 	if fails
> 		tries the libnl(1)
> 		if fails
> 			prints warning
> 
> --with-libnl=libnl3
> 	tries the libnl3
> 	if fails
> 		prints error and fails
> 
> --with-libnl=libnl1
> 	tries the libnl(1)
> 	if fails
> 		prints error and fails
> 
> --with-libnl=no / --without-libnl
> 	libnl disabled by default
> 
> >Why do you check for libnl-cli here? That's the command line tools..
> >
> >Also is there any reason to require 3.2 as the minimum version? Was
> >there any feature present in 3.2 upstream that would prevent us from
> >using 3.0 or 3.1 ?
> 
> This was because the check for libnl-cli exports the correct _LIBS
> and _CFLAGS, replaced with only the needed components
> 
> >>+#define nlw_disable_seq_check   nl_socket_disable_seq_check
> >>+
> >>+#define nlw_geterror            nl_geterror
> >>+
> >>+#define HAVE_NL_SET_PASSCRED 1
> >>+#define HAVE_NL_SOCKET_ADD_MEMBERSHIP 1
> >>+#define HAVE_NL_SOCKET_MODIFY_CB 1
> >>+
> >>+#else /* LIBNL1 */
> >>+
> >
> >Maybe we should be paranoid here, check for HAVE_LIBNL1 in this branch
> >and simply #error out in the #else branch.
> >
> >>+#define nlw_destroy_handle      nl_handle_destroy
> >>+#define nlw_alloc               nl_handle_alloc
> >>+#define nlw_handle              nl_handle
> >>+#define nlw_disable_seq_check   nl_disable_sequence_check
> >>+
> >>+#define nlw_geterror(error) nl_geterror()
> >
> >This seems strange, the error variable is not passed on to the
> >nl_geterror. Also nlw_geterror expands to nl_geterror for both libnl1
> >and libnl3.
> >
> 
> This is because the nl_geterror changet between versions. In libnl3
> it takes an argument representing error message number, while in
> libnl1 it had no argument at all.
> 

Ah, I see, they used a global errno in libnl1. Terrible :)

> >Why did you rename the handle to nlp? It's not wrong, I just don't see
> >the point, sometimes the variable name change is the only change in
> >certain hunks.
> 
> It seemed better than keep it nlh (h for handle) while we are using
> the nl_handle or nl_socket depending or netlink version. So i set it
> to nlp - netlink pointer, it seemed appropriate. Should i change it
> back?
> 
> 

I don't really mind.

This is better, almost there. See a couple of comments inline.

> New patch is attached.
> 
> Ondra

> From 6a6ca5226a2988bbd5f828de0b9b0c67ec116b4a Mon Sep 17 00:00:00 2001
> From: Ondrej Kos <okos at redhat.com>
> Date: Mon, 18 Feb 2013 14:43:19 +0100
> Subject: [PATCH] Provide libnl3 support
> 
> https://fedorahosted.org/sssd/ticket/812
> 
> Update the monitor code to be using the new libnl3 API.
> 
> Changed configure option
> --with-libnl
> 
> By default, it tries to build with libnl3, if not found, then with
> libnl1, if this isn't found either, build proceeds without libnl, just
> with warning.
> 
> Specifing --with-libnl=<libnl3|libnl1|no> checks for the specific given
> version, if not found, configure ends with error.
> ---
>  BUILD.txt                     |   2 +-
>  configure.ac                  |   8 +--
>  src/conf_macros.m4            |  31 ++++++++-
>  src/external/libnl.m4         |  14 +++-
>  src/external/libnl3.m4        |  27 ++++++++
>  src/monitor/monitor_netlink.c | 158 +++++++++++++++++++++++++++++-------------
>  6 files changed, 178 insertions(+), 62 deletions(-)
>  create mode 100644 src/external/libnl3.m4
> 
> diff --git a/BUILD.txt b/BUILD.txt
> index 0eb3f468c14eb213a97a1522334f43653c80ff75..263129215d6af43869a9d97bbd291436b2cc914d 100644
> --- a/BUILD.txt
> +++ b/BUILD.txt
> @@ -26,7 +26,7 @@ yum install openldap-devel gettext libtool pcre-devel c-ares-devel \
>      libxml2 pam-devel nss-devel libtevent python-devel \
>      libtevent-devel libtdb libtdb-devel libtalloc libtalloc-devel \
>      libldb libldb-devel popt-devel c-ares-devel check-devel \
> -    doxygen libselinux-devel libsemanage-devel bind-utils libnl-devel \
> +    doxygen libselinux-devel libsemanage-devel bind-utils libnl3-devel \
>      gettext-devel glib2-devel
>  
>  ding-libs are available in Fedora 14 and later version:
> diff --git a/configure.ac b/configure.ac
> index 4b1cfba26166bbe407ce219d8ad86e8b1a94c88b..8bb2e0f526f06a84546afe9b12a0eb4f0dee9fb3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -114,7 +114,6 @@ WITH_PYTHON_BINDINGS
>  WITH_SELINUX
>  WITH_NSCD
>  WITH_SEMANAGE
> -WITH_LIBNL
>  WITH_NOLOGIN_SHELL
>  WITH_APP_LIBS
>  WITH_SUDO
> @@ -147,6 +146,7 @@ m4_include([src/external/nscd.m4])
>  m4_include([src/external/nsupdate.m4])
>  m4_include([src/external/libkeyutils.m4])
>  m4_include([src/external/libnl.m4])
> +m4_include([src/external/libnl3.m4])

I still don't understand why you separated libnl.m4 and libnl3.m4. Can
you merge them? They only contain function definitions with AC_DEFUN. We
don't have separate files for separate krb5 libraries for example.

>  m4_include([src/external/systemd.m4])
>  m4_include([src/external/pac_responder.m4])
>  m4_include([src/util/signal.m4])

<snip>

> diff --git a/src/external/libnl.m4 b/src/external/libnl.m4
> index 36619ff87490fa94651255224b9fb57e3194ecbc..1818065c8ebcf222bd4558d1437faf25d0e9fc8e 100644
> --- a/src/external/libnl.m4
> +++ b/src/external/libnl.m4
> @@ -1,11 +1,19 @@
>  dnl A macro to check the availability and version of libnetlink
>  
> -AC_DEFUN([AM_CHECK_LIBNL],
> +AC_DEFUN([AM_CHECK_LIBNL1],
>  [
> -    PKG_CHECK_MODULES(libnl, libnl-1 >= 1.1,[
> +    PKG_CHECK_MODULES(LIBNL, libnl-1 >= 1.1,[
> +
>          HAVE_LIBNL=1
> +        HAVE_LIBNL1=1
> +
>          AC_SUBST(HAVE_LIBNL)
> +        AC_SUBST(HAVE_LIBNL1)

I know the AC_SUBST(HAVE_LIBNL) was there even previously, but I can't
find any @HAVE_LIBNL@ anywhere. Does it actually serve any purpose?
Same for HAVE_LIBNL1 and HAVE_LIBNL3 substitutions.

> +
>          AC_DEFINE_UNQUOTED(HAVE_LIBNL, 1, [Build with libnetlink support])
> +        AC_DEFINE_UNQUOTED(HAVE_LIBNL1, 1, [Libnetlink version = 1])
> +
> +        AC_MSG_NOTICE([Building with libnl])
>  
>  	    AC_CHECK_HEADERS(netlink.h)
>  	    AC_CHECK_LIB(nl, nl_connect, [ LIBNL_LIBS="-lnl" ], [AC_MSG_ERROR([libnl is required])])
> @@ -28,7 +36,7 @@ AC_DEFUN([AM_CHECK_LIBNL],
>  	                 [AC_DEFINE([HAVE_NL_SET_PASSCRED], 1, [Does libnl have nl_set_passcred?])
>  	                 ],
>  	    )
> -    ],[AC_MSG_WARN([Netlink support unavailable or too old])])
> +    ],[AC_MSG_WARN([Netlink v1 support unavailable or too old])])
>  
>      AC_SUBST(LIBNL_CFLAGS)
>      AC_SUBST(LIBNL_LIBS)
> diff --git a/src/external/libnl3.m4 b/src/external/libnl3.m4
> new file mode 100644
> index 0000000000000000000000000000000000000000..c2dda2ace1e4f193952560c0cbcd095f86c8273d
> --- /dev/null
> +++ b/src/external/libnl3.m4
> @@ -0,0 +1,27 @@
> +dnl A macro to check the availability of libnetlink version 3
> +
> +AC_DEFUN([AM_CHECK_LIBNL3],
> +[
> +    PKG_CHECK_MODULES(LIBNL, [
> +        libnl-3.0 >= 3.0
> +dnl        libnl-genl-3.0 >= 3.0

You probably want to remove the commented-out line.

> +        libnl-nf-3.0 >= 3.0

Do we use anything from netfilter now? I don't see any nf_ identifiers
in the code.

> +        libnl-route-3.0 >= 3.0], [
> +
> +        HAVE_LIBNL=1
> +        HAVE_LIBNL3=1
> +
> +        AC_SUBST(HAVE_LIBNL)
> +        AC_SUBST(HAVE_LIBNL3)

Same comment about AC_SUBST as above

> +
> +        AC_DEFINE_UNQUOTED(HAVE_LIBNL, 1, [Build with libnetlink support])
> +        AC_DEFINE_UNQUOTED(HAVE_LIBNL3, 1, [Libnetlink version = 3])
> +
> +        AC_MSG_NOTICE([Building with libnl3])
> +
> +        AC_CHECK_HEADERS(netlink/netlink.h)
> +    ],[AC_MSG_WARN([Netlink v3 support unavailable or too old])])
> +
> +    AC_SUBST(LIBNL_CFLAGS)
> +    AC_SUBST(LIBNL_LIBS)
> +])
> diff --git a/src/monitor/monitor_netlink.c b/src/monitor/monitor_netlink.c
> index 3842c4f1612367ab2d33a1000d81fd29506fbf4d..55d122b1b515ea19e34d8d773d685adb3c76e8fe 100644
> --- a/src/monitor/monitor_netlink.c
> +++ b/src/monitor/monitor_netlink.c
> @@ -47,6 +47,7 @@
>  #include <netlink/route/rtnl.h>
>  #include <netlink/route/route.h>
>  #include <netlink/handlers.h>
> +#include <netlink/socket.h>
>  #endif
>  
>  /* Linux header file confusion causes this to be undefined. */
> @@ -54,12 +55,6 @@
>  #define SOL_NETLINK 270
>  #endif
>  
> -#define nlw_get_fd nl_socket_get_fd
> -#define nlw_recvmsgs_default nl_recvmsgs_default
> -#define nlw_get_pid nl_socket_get_local_port
> -#define nlw_object_match nl_object_match_filter
> -#define NLW_OK NL_OK
> -
>  #define SYSFS_IFACE_TEMPLATE "/sys/class/net/%s"
>  #define SYSFS_IFACE_PATH_MAX (16+IFNAMSIZ)
>  
> @@ -73,6 +68,40 @@
>  
>  #define BUFSIZE 8
>  
> +#ifdef HAVE_LIBNL
> +/* Wrappers determining use of libnl version 1 or 3 */
> +#ifdef HAVE_LIBNL3
> +
> +#define nlw_destroy_handle      nl_socket_free
> +#define nlw_alloc               nl_socket_alloc
> +#define nlw_set_passcred        nl_socket_set_passcred
> +#define nlw_disable_seq_check   nl_socket_disable_seq_check
> +#define nlw_route_get_oif       rtnlw_route_get_oif
> +
> +#define nlw_geterror(error)     nl_geterror(error)
> +
> +#define nlw_handle              nl_sock
> +
> +#define HAVE_NL_SET_PASSCRED 1
> +#define HAVE_NL_SOCKET_ADD_MEMBERSHIP 1
> +#define HAVE_NL_SOCKET_MODIFY_CB 1
> +

Sorry, I missed this in the first review. The HAVE_ constanst should
never be declared directly, I think, only by configure.

The checks that are currently
in external/libnl.m4 in the libnl1 branch only should probably be moved
into a branch that is taken if either libnl is there and
rtnl_route_get_oif. Then you could check for HAVE_RTNL_ROUTE_GET_OIF
instead of just HAVE_LIBNL3.

> +#elif HAVE_LIBNL1
> +
> +#define nlw_destroy_handle      nl_handle_destroy
> +#define nlw_alloc               nl_handle_alloc
> +#define nlw_set_passcred        nl_set_passcred
> +#define nlw_disable_seq_check   nl_disable_sequence_check
> +#define nlw_route_get_oif       rtnl_route_get_oif
> +
> +#define nlw_geterror(error)     nl_geterror()
> +
> +#define nlw_handle              nl_handle
> +
> +#endif /* HAVE_LIBNL3 */
> +
> +#endif /* HAVE_LIBNL */
> +
>  enum nlw_msg_type {
>      NLW_LINK,
>      NLW_ROUTE,
> @@ -82,7 +111,7 @@ enum nlw_msg_type {
>  
>  struct netlink_ctx {
>  #ifdef HAVE_LIBNL
> -    struct nl_handle *nlh;
> +    struct nlw_handle *nlp;
>  #endif
>      struct tevent_fd *tefd;
>  
> @@ -96,7 +125,7 @@ static int netlink_ctx_destructor(void *ptr)
>      struct netlink_ctx *nlctx;
>      nlctx = talloc_get_type(ptr, struct netlink_ctx);
>  
> -    nl_handle_destroy(nlctx->nlh);
> +    nlw_destroy_handle(nlctx->nlp);
>      return 0;
>  }
>  
> @@ -104,6 +133,25 @@ static int netlink_ctx_destructor(void *ptr)
>   *                      Utility functions
>   *******************************************************************/
>  
> +#ifdef HAVE_LIBNL3

See above.

> +/* rtnl_route_get_oif removed from libnl3 */
> +int
> +rtnlw_route_get_oif(struct rtnl_route * route)
> +{
> +	struct rtnl_nexthop * nh;
> +	int hops;
> +
> +	hops = rtnl_route_get_nnexthops(route);
> +    if (hops <= 0) {

Indentation..

> +        return 0;
> +    }
> +
> +	nh = rtnl_route_nexthop_n(route, 0);
> +
> +	return rtnl_route_nh_get_ifindex(nh);
> +}
> +#endif
> +
>  static bool has_wireless_extension(const char *ifname)
>  {
>      int s;

<snip>

> @@ -699,17 +751,21 @@ int setup_netlink(TALLOC_CTX *mem_ctx, struct tevent_context *ev,
>      nlctx->change_cb = change_cb;
>      nlctx->cb_data   = cb_data;
>  
> -    /* allocate the libnl handle and register the default filter set */
> -    nlctx->nlh = nl_handle_alloc();
> -    if (!nlctx->nlh) {
> -        DEBUG(1, (("unable to allocate netlink handle: %s"),
> -                   nl_geterror()));
> +    /* allocate the libnl handle/socket and register the default filter set */
> +    nlctx->nlp = nlw_alloc();
> +    if (!nlctx->nlp) {
> +#ifdef HAVE_LIBNL1
> +        DEBUG(SSSDBG_CRIT_FAILURE, (("unable to allocate netlink handle: %s"),
> +                                     nl_geterror()));
> +#elif HAVE_LIBNL3
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("unable to allocate netlink socket\n"));
> +#endif

You use just one branch here and ret (set to ENOMEM) if allocation failed. No
need to #ifdef for an error message,



More information about the sssd-devel mailing list