[SSSD] [PATCH] Provide libnl3 support

Ondrej Kos okos at redhat.com
Tue Mar 26 14:12:24 UTC 2013


On 03/22/2013 02:19 PM, Jakub Hrozek wrote:
> 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-Definitions.html
>
>> +        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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Provide-libnl3-support.patch
Type: text/x-patch
Size: 19594 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130326/b9ae9ef8/attachment.bin>


More information about the sssd-devel mailing list