[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