[SSSD] [PATCH] Provide libnl3 support

Ondrej Kos okos at redhat.com
Fri Mar 22 09:57:18 UTC 2013


On 03/21/2013 05:46 PM, Jakub Hrozek wrote:
> On Thu, Mar 21, 2013 at 05:21:09PM +0100, Jakub Hrozek wrote:
>> 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.
>
> Also please amend the specfile so that for F-18 and higher, libnl3 is
> BuildRequired.

Hi,

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: 19754 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130322/a581aeb0/attachment.bin>


More information about the sssd-devel mailing list