On Fri, Mar 22, 2013 at 10:57:18AM +0100, Ondrej Kos wrote:
> Hi,
>
> Thanks for the review, new patch is attached.
Almost there. See inline for a couple of last requests.
> --- a/contrib/sssd.spec.in
> +++ b/contrib/sssd.spec.in
> @@ -122,14 +122,17 @@ BuildRequires: libselinux-devel
> BuildRequires: libsemanage-devel
> BuildRequires: bind-utils
> BuildRequires: keyutils-libs-devel
> -BuildRequires: libnl-devel
> BuildRequires: gettext-devel
> BuildRequires: pkgconfig
> BuildRequires: findutils
> BuildRequires: glib2-devel
> BuildRequires: selinux-policy-targeted
> +%if (0%{?fedora} < 18)
> +BuildRequires: libnl-devel
> +%endif
> %if (0%{?fedora} >= 18)
> BuildRequires: libcmocka-devel
> +BuildRequires: libnl3-devel
> %endif
>
I think this would mean that RHEL would be build without libnl
completely because it would match neither if?
> index
36619ff87490fa94651255224b9fb57e3194ecbc..1ccc4308a6f776f0f2241a9e54c472a193493c2a 100644
> --- a/src/external/libnl.m4
> +++ b/src/external/libnl.m4
> @@ -1,34 +1,88 @@
> +dnl A macro to check if this particular version of libnl supports particular common
libnl functions
> +AC_DEFUN([AM_CHECK_LIBNL_FCS],
> +[
> + if test x$LNL_CHECK_LIB != x; then
> +
Please pass function parameter instead of passing the info via a global
variable:
http://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Macro...
> + AC_CHECK_LIB($LNL_CHECK_LIB,
> + [nl_socket_add_membership],
> + [AC_DEFINE([HAVE_NL_SOCKET_ADD_MEMBERSHIP], 1, [Does libnl have
nl_socket_add_membership?])
> + ],
> + )
> +
> + AC_CHECK_LIB($LNL_CHECK_LIB,
> + [nl_socket_modify_cb],
> + [AC_DEFINE([HAVE_NL_SOCKET_MODIFY_CB], 1, [Does libnl have
nl_socket_modify_cb?])
> + ],
> + )
> +
> + AC_CHECK_LIB($LNL_CHECK_LIB,
> + [rtnl_route_get_oif],
> + [AC_DEFINE([HAVE_RTNL_ROUTE_GET_OIF], 1, [Does libnl have
rtnl_route_get_oif?])
> + ],
> + )
> +
> + fi
> +])
> +
> dnl A macro to check the availability and version of libnetlink
> +AC_DEFUN([AM_CHECK_LIBNL1],
> +[
> + PKG_CHECK_MODULES(LIBNL, libnl-1 >= 1.1,[
> +
> + HAVE_LIBNL=1
> + HAVE_LIBNL1=1
> +
> + LNL_CHECK_LIB=nl
The value of this variable would be just passed to the
AM_CHECK_LIBNL_FCS macro as parameter.
> AC_SUBST(LIBNL_CFLAGS)
> AC_SUBST(LIBNL_LIBS)
> diff --git a/src/monitor/monitor_netlink.c b/src/monitor/monitor_netlink.c
> index
3842c4f1612367ab2d33a1000d81fd29506fbf4d..d7d1a341435852e14553421f7b2f48e513a93671 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,36 @@
>
> #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
No need for the indirect wrapper see below.
> @@ -104,6 +129,25 @@ static int netlink_ctx_destructor(void *ptr)
> * Utility functions
> *******************************************************************/
>
> +#ifndef HAVE_RTNL_ROUTE_GET_OIF
> +/* 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) {
> + return 0;
> + }
> +
> + nh = rtnl_route_nexthop_n(route, 0);
> +
> + return rtnl_route_nh_get_ifindex(nh);
> +}
You can always call this wrapper in the code, move the #ifdef inside the
function and call rtnl_route_get_oif() directly in the #else case. That
would get rid of the double indirection via wrapper and then with macro.
The rest looks good to me.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for the review, new patch is attached
Ondra
--
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech
phone: +420-532-294-558
cell: +420-736-417-909
ext: 82-62558
loc: 1013 Brno 1 office
irc: okos @ #sssd #brno