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.