[PATCHES] DYNDNS: support mult. interfaces for dyndns_iface opt

[Pavel Reichl]

Hello,

please see attached patches.

1st patch adds return value ENOENT to sss_iface_addr_list_get() so I can 
provide more concrete debug message for missing interface or if 
interface is not suitable (missing IP address)

2nd patch:
* I introduced new public function sss_iface_addr_concatenate(), I'm 
aware that this function is probably not reusable but I needed to work 
around that 'struct sss_iface_addr' in defined in source file only.
* I had troubles with correctly handling creating talloc hiearchy of IPs 
of different interfaces. I decided to use first address of first found 
interface as a parent talloc context for other interfaces. I attached 
talloc report output to illustrate this.
> full talloc report on 'struct sdap_dyndns_get_addrs_state' (total
>      16 bytes in   1 blocks)
> full talloc report on 'struct sdap_dyndns_get_addrs_state' (total
>     376 bytes in  19 blocks)
>  struct sss_iface_addr          contains    360 bytes in  18
> blocks (ref 0) 0xbc0650
> struct sss_iface_addr          contains    120 bytes in   6 blocks
>  (ref 0) 0xbecee0
> struct sss_iface_addr          contains     80 bytes in   4 blocks
>  (ref 0) 0xbeb920
>  struct sss_iface_addr          contains     40 bytes in   2
> blocks (ref 0) 0xbd03f0
>      ../src/providers/dp_dyndns.c:219 contains     16 bytes in
> 1 blocks (ref 0) 0xbd0470
>  ../src/providers/dp_dyndns.c:219 contains     16 bytes in   1
> blocks (ref 0) 0xbeb9a0
> ../src/providers/dp_dyndns.c:219 contains     16 bytes in 1 blocks
>  (ref 0) 0xbecf60
> struct sss_iface_addr          contains    120 bytes in   6 blocks
> (ref 0) 0xbd0640
> struct sss_iface_addr          contains     80 bytes in   4 blocks
> (ref 0) 0xbd19a0
> struct sss_iface_addr          contains     40 bytes in   2
> blocks (ref 0) 0xbcfb00
>     ../src/providers/dp_dyndns.c:219 contains     16 bytes in
> 1 blocks (ref 0) 0xbed300
> ../src/providers/dp_dyndns.c:219 contains     16 bytes in   1
> blocks (ref 0) 0xbd1a20
> ../src/providers/dp_dyndns.c:219 contains     16 bytes in 1 blocks
> (ref 0) 0xbd06c0
> struct sss_iface_addr          contains     80 bytes in   4 blocks
> (ref 0) 0xbd0eb0
> struct sss_iface_addr          contains     40 bytes in   2 blocks
> (ref 0) 0xbd1900
> ../src/providers/dp_dyndns.c:219 contains     16 bytes in   1
> blocks (ref 0) 0xbec4f0
> ../src/providers/dp_dyndns.c:219 contains     16 bytes in 1 blocks
> (ref 0) 0xbeca10
> ../src/providers/dp_dyndns.c:219 contains     16 bytes in 1 blocks
> (ref 0) 0xbe6ae0

* I was thinking whether it would be a good idea to handle the case when 
processing of interfaces provided in dyndns_iface yields no address at 
all by continuing to detect DYNDNS address from LDAP connection?

Thanks!

[Pavel Reichl]

On 07/09/2015 11:15 AM, Pavel Reichl wrote:
> Hello,
>
> please see attached patches.
>
> 1st patch adds return value ENOENT to sss_iface_addr_list_get() so I 
> can provide more concrete debug message for missing interface or if 
> interface is not suitable (missing IP address)
>
> 2nd patch:
> I introduced new public function sss_iface_addr_concatenate(), I'm
>
> aware that this function is probably not reusable but I needed to work 
> around that 'struct sss_iface_addr' in defined in source file only.
> I had troubles with correctly handling creating talloc hiearchy of
>
> IPs of different interfaces. I decided to use first address of first 
> found interface as a parent talloc context for other interfaces. I 
> attached talloc report output to illustrate this.
> > full talloc report on 'struct sdap_dyndns_get_addrs_state' (total
> >      16 bytes in   1 blocks)
> > full talloc report on 'struct sdap_dyndns_get_addrs_state' (total
> >     376 bytes in  19 blocks)
> > struct sss_iface_addr          contains    360 bytes in  18
> >  blocks (ref 0) 0xbc0650
> >    struct sss_iface_addr          contains    120 bytes in   6
> > blocks (ref 0) 0xbecee0
> >        struct sss_iface_addr          contains     80 bytes in
> > 4 blocks (ref 0) 0xbeb920
> >            struct sss_iface_addr          contains 40 bytes in
> > 2 blocks (ref 0) 0xbd03f0
> >                ../src/providers/dp_dyndns.c:219 contains    
> > 16 bytes in   1 blocks (ref 0) 0xbd0470
> >            ../src/providers/dp_dyndns.c:219 contains   16
> > bytes in   1 blocks (ref 0) 0xbeb9a0
> >        ../src/providers/dp_dyndns.c:219 contains     16 bytes
> > in   1 blocks (ref 0) 0xbecf60
> >   struct sss_iface_addr          contains    120 bytes in   6
> > blocks (ref 0) 0xbd0640
> >       struct sss_iface_addr          contains     80 bytes in
> > 4 blocks (ref 0) 0xbd19a0
> >           struct sss_iface_addr          contains 40 bytes in
> > 2 blocks (ref 0) 0xbcfb00
> >               ../src/providers/dp_dyndns.c:219 contains    
> > 16 bytes in   1 blocks (ref 0) 0xbed300
> >           ../src/providers/dp_dyndns.c:219 contains   16
> > bytes in   1 blocks (ref 0) 0xbd1a20
> >       ../src/providers/dp_dyndns.c:219 contains     16 bytes
> > in   1 blocks (ref 0) 0xbd06c0
> >   struct sss_iface_addr          contains     80 bytes in   4
> > blocks (ref 0) 0xbd0eb0
> >       struct sss_iface_addr          contains     40 bytes in
> > 2 blocks (ref 0) 0xbd1900
> >           ../src/providers/dp_dyndns.c:219 contains   16
> > bytes in   1 blocks (ref 0) 0xbec4f0
> >       ../src/providers/dp_dyndns.c:219 contains     16 bytes
> > in   1 blocks (ref 0) 0xbeca10
> >   ../src/providers/dp_dyndns.c:219 contains     16 bytes in  
> > 1 blocks (ref 0) 0xbe6ae0
> I was thinking whether it would be a good idea to handle the case
>
> when processing of interfaces provided in dyndns_iface yields no 
> address at all by continuing to detect DYNDNS address from LDAP 
> connection?
>
> Thanks!
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

While reading #2558 I noticed that there is a further request relating 
to these patches - mbasti asks If there could be some mean how to send 
IPs of all interfaces?
Is this a good idea in general?
I suppose I could implement it with some special value for 
'dyndns_iface' - ideally the special value would contain characters 
prohibited to be part of interface name if there are such...

[Jakub Hrozek]

On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
> While reading #2558 I noticed that there is a further request relating to
> these patches - mbasti asks If there could be some mean how to send IPs of
> all interfaces?
> Is this a good idea in general?
> I suppose I could implement it with some special value for 'dyndns_iface' -
> ideally the special value would contain characters prohibited to be part of
> interface name if there are such...

I would say this looks like a useful feature in general, but can you ask
Martin why he needs this feature? I think the time has come for 1.13
development to start turning into bug fixing more, so unless there is
really some particular use-case, please move the ticket into 1.14.

[Martin Basti]

On 10/07/15 16:55, Jakub Hrozek wrote:
> On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
> > While reading #2558 I noticed that there is a further request relating to
> > these patches - mbasti asks If there could be some mean how to send IPs of
> > all interfaces?
> > Is this a good idea in general?
> > I suppose I could implement it with some special value for 'dyndns_iface' -
> > ideally the special value would contain characters prohibited to be part of
> > interface name if there are such...
>
> I would say this looks like a useful feature in general, but can you ask
> Martin why he needs this feature? I think the time has come for 1.13
> development to start turning into bug fixing more, so unless there is
> really some particular use-case, please move the ticket into 1.14.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

This is blocker for https://fedorahosted.org/freeipa/ticket/4249

Without this patch we cannot add dualstack and  multihomed support to 
IPA clients, because when IPA adds all IP and IPv6 addresses to DNS, 
then SSSD just deletes all records and put there just one address.

We want to support dualstack in IPA for long time. Several users wanted 
dualstack, even multihomed support.

Martin2

[Jakub Hrozek]

On Fri, Jul 10, 2015 at 05:52:20PM +0200, Martin Basti wrote:
> On 10/07/15 16:55, Jakub Hrozek wrote:
> > On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
> > > While reading #2558 I noticed that there is a further request relating to
> > > these patches - mbasti asks If there could be some mean how to send IPs of
> > > all interfaces?
> > > Is this a good idea in general?
> > > I suppose I could implement it with some special value for 'dyndns_iface' -
> > > ideally the special value would contain characters prohibited to be part of
> > > interface name if there are such...
> >
> > I would say this looks like a useful feature in general, but can you ask
> > Martin why he needs this feature? I think the time has come for 1.13
> > development to start turning into bug fixing more, so unless there is
> > really some particular use-case, please move the ticket into 1.14.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> This is blocker for https://fedorahosted.org/freeipa/ticket/4249
>
> Without this patch we cannot add dualstack and  multihomed support to IPA
> clients, because when IPA adds all IP and IPv6 addresses to DNS, then SSSD
> just deletes all records and put there just one address.

I can see the argument for dualstack -- we shouldn't remove v6 addresses
if we're talking to the server over v4 and conversely. I suspect the
multi-homed use-case is a bit less important, right?
> We want to support dualstack in IPA for long time. Several users wanted
> dualstack, even multihomed support.

Is there a corresponding IPA ticket or some link to a freeipa-users
thread?

Please note I agree with the technical requirement, but there's limited
time, so we should prioritize.

[Martin Basti]

On 10/07/15 20:16, Jakub Hrozek wrote:
> On Fri, Jul 10, 2015 at 05:52:20PM +0200, Martin Basti wrote:
> > On 10/07/15 16:55, Jakub Hrozek wrote:
> > > On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
> > > > While reading #2558 I noticed that there is a further request relating to
> > > > these patches - mbasti asks If there could be some mean how to send IPs of
> > > > all interfaces?
> > > > Is this a good idea in general?
> > > > I suppose I could implement it with some special value for 'dyndns_iface' -
> > > > ideally the special value would contain characters prohibited to be part of
> > > > interface name if there are such...
> > >
> > > I would say this looks like a useful feature in general, but can you ask
> > > Martin why he needs this feature? I think the time has come for 1.13
> > > development to start turning into bug fixing more, so unless there is
> > > really some particular use-case, please move the ticket into 1.14.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > This is blocker for https://fedorahosted.org/freeipa/ticket/4249
> >
> > Without this patch we cannot add dualstack and  multihomed support to IPA
> > clients, because when IPA adds all IP and IPv6 addresses to DNS, then SSSD
> > just deletes all records and put there just one address.
>
> I can see the argument for dualstack -- we shouldn't remove v6 addresses
> if we're talking to the server over v4 and conversely. I suspect the
> multi-homed use-case is a bit less important, right?

Yes multihomed support is less important.
> > We want to support dualstack in IPA for long time. Several users wanted
> > dualstack, even multihomed support.
>
> Is there a corresponding IPA ticket or some link to a freeipa-users
> thread?
>
> Please note I agree with the technical requirement, but there's limited
> time, so we should prioritize.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

IPv6 IPA tickets:

https://fedorahosted.org/freeipa/query?keywords=~ipv6&status=!closed

Martin^2

[Pavel Reichl]

On 07/13/2015 12:04 PM, Martin Basti wrote:
> On 10/07/15 20:16, Jakub Hrozek wrote:
> > On Fri, Jul 10, 2015 at 05:52:20PM +0200, Martin Basti wrote:
> > > On 10/07/15 16:55, Jakub Hrozek wrote:
> > > > On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
> > > > > While reading #2558 I noticed that there is a further request 
> > > > > relating to
> > > > > these patches - mbasti asks If there could be some mean how to 
> > > > > send IPs of
> > > > > all interfaces?
> > > > > Is this a good idea in general?
> > > > > I suppose I could implement it with some special value for 
> > > > > 'dyndns_iface' -
> > > > > ideally the special value would contain characters prohibited to 
> > > > > be part of
> > > > > interface name if there are such...
> > > >
> > > > I would say this looks like a useful feature in general, but can 
> > > > you ask
> > > > Martin why he needs this feature? I think the time has come for 1.13
> > > > development to start turning into bug fixing more, so unless there is
> > > > really some particular use-case, please move the ticket into 1.14.
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > This is blocker for https://fedorahosted.org/freeipa/ticket/4249
> > >
> > > Without this patch we cannot add dualstack and  multihomed support 
> > > to IPA
> > > clients, because when IPA adds all IP and IPv6 addresses to DNS, 
> > > then SSSD
> > > just deletes all records and put there just one address.
> >
> > I can see the argument for dualstack -- we shouldn't remove v6 addresses
> > if we're talking to the server over v4 and conversely. I suspect the
> > multi-homed use-case is a bit less important, right?
>
> Yes multihomed support is less important.
> > > We want to support dualstack in IPA for long time. Several users wanted
> > > dualstack, even multihomed support.
> >
> > Is there a corresponding IPA ticket or some link to a freeipa-users
> > thread?
> >
> > Please note I agree with the technical requirement, but there's limited
> > time, so we should prioritize.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> IPv6 IPA tickets:
>
> https://fedorahosted.org/freeipa/query?keywords=~ipv6&status=!closed
>
> Martin^2
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hello, please see updated patch set, 1st and 2nd patch were not changed 
(except for the ticket number in commit message).

I have tested the patches against IPA successfully, patches also work 
against AD but I'm seeing some problems in log - I have to investigate 
this thoroughly but I believe that the problem is rather 
miscommunication of AD then bug in my patches.

Thanks!

[Pavel Reichl]

On 07/15/2015 07:40 PM, Pavel Reichl wrote:
> [snip]
> Hello, please see updated patch set, 1st and 2nd patch were not 
> changed (except for the ticket number in commit message).
>
> I have tested the patches against IPA successfully, patches also work 
> against AD but I'm seeing some problems in log - I have to investigate 
> this thoroughly but I believe that the problem is rather 
> miscommunication of AD then bug in my patches.

Thanks to help of Petr Spacek I was able to fix my AD configuration so 
my tests in AD and IPA were both successful!
> Thanks!
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Martin Basti]

On 16/07/15 11:01, Pavel Reichl wrote:
> On 07/15/2015 07:40 PM, Pavel Reichl wrote:
> > [snip]
> > Hello, please see updated patch set, 1st and 2nd patch were not 
> > changed (except for the ticket number in commit message).
> >
> > I have tested the patches against IPA successfully, patches also work 
> > against AD but I'm seeing some problems in log - I have to 
> > investigate this thoroughly but I believe that the problem is rather 
> > miscommunication of AD then bug in my patches.
>
> Thanks to help of Petr Spacek I was able to fix my AD configuration so 
> my tests in AD and IPA were both successful!
> > Thanks!
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Works for me, Pavel is God.

[Jakub Hrozek]

On Wed, Jul 15, 2015 at 07:40:29PM +0200, Pavel Reichl wrote:
> On 07/13/2015 12:04 PM, Martin Basti wrote:
> > On 10/07/15 20:16, Jakub Hrozek wrote:
> > > On Fri, Jul 10, 2015 at 05:52:20PM +0200, Martin Basti wrote:
> > > > On 10/07/15 16:55, Jakub Hrozek wrote:
> > > > > On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
> > > > > > While reading #2558 I noticed that there is a further request
> > > > > > relating to
> > > > > > these patches - mbasti asks If there could be some mean how to
> > > > > > send IPs of
> > > > > > all interfaces?
> > > > > > Is this a good idea in general?
> > > > > > I suppose I could implement it with some special value for
> > > > > > 'dyndns_iface' -
> > > > > > ideally the special value would contain characters prohibited to
> > > > > > be part of
> > > > > > interface name if there are such...
> > > > >
> > > > > I would say this looks like a useful feature in general, but can you
> > > > > ask
> > > > > Martin why he needs this feature? I think the time has come for 1.13
> > > > > development to start turning into bug fixing more, so unless there is
> > > > > really some particular use-case, please move the ticket into 1.14.
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > This is blocker for https://fedorahosted.org/freeipa/ticket/4249
> > > >
> > > > Without this patch we cannot add dualstack and  multihomed support to
> > > > IPA
> > > > clients, because when IPA adds all IP and IPv6 addresses to DNS, then
> > > > SSSD
> > > > just deletes all records and put there just one address.
> > >
> > > I can see the argument for dualstack -- we shouldn't remove v6 addresses
> > > if we're talking to the server over v4 and conversely. I suspect the
> > > multi-homed use-case is a bit less important, right?
> >
> > Yes multihomed support is less important.
> > > > We want to support dualstack in IPA for long time. Several users wanted
> > > > dualstack, even multihomed support.
> > >
> > > Is there a corresponding IPA ticket or some link to a freeipa-users
> > > thread?
> > >
> > > Please note I agree with the technical requirement, but there's limited
> > > time, so we should prioritize.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > IPv6 IPA tickets:
> >
> > https://fedorahosted.org/freeipa/query?keywords=~ipv6&status=!closed
> >
> > Martin^2
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Hello, please see updated patch set, 1st and 2nd patch were not changed
> (except for the ticket number in commit message).
>
> I have tested the patches against IPA successfully, patches also work
> against AD but I'm seeing some problems in log - I have to investigate this
> thoroughly but I believe that the problem is rather miscommunication of AD
> then bug in my patches.
>
> Thanks!

This is mostly a code review as Martin Basti already did some testing.
> From ad26074e4c569ed32eeb40ae950a48db489d7c5c Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 8 Jul 2015 09:01:24 -0400
> Subject: [PATCH 1/5] DYNDNS: sss_iface_addr_list_get return ENOENT
>
> If none of eligible interfaces matches ifname then ENOENT is returned.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2549
>
> src/providers/dp_dyndns.c        |  8 +++++++-
>  src/providers/ldap/sdap_dyndns.c |  4 ++++
>  src/tests/cmocka/test_dyndns.c   | 20 ++++++++++++++++++++
>  3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> index 1cac3d0fae2454cea823ed640a4325f27580353f..9552e83d8d4ba615c157c9bae275c8ab867ec274 100644
> --- a/src/providers/dp_dyndns.c
> +++ b/src/providers/dp_dyndns.c
> @@ -222,7 +222,13 @@ sss_iface_addr_list_get(TALLOC_CTX *mem_ctx, const char *ifname,
>          }
>      }
> ret = EOK;
> if (addrlist != NULL) {
>    /* OK, some result was found */
>    ret = EOK;
> } else {
>    /* No result was found */
>    ret = ENOENT;

I think we should have some non-verbose DEBUG message here.
> }
> *_addrlist = addrlist;
>
> done:
>      freeifaddrs(ifaces);
> diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
> index 0d9c9205792062378aa25aad6ac706058001433a..bb98f8e1ec6852b04e9ea3e8d5807fecc7e6958d 100644
> --- a/src/providers/ldap/sdap_dyndns.c
> +++ b/src/providers/ldap/sdap_dyndns.c
> @@ -504,6 +504,10 @@ sdap_dyndns_get_addrs_send(TALLOC_CTX *mem_ctx,
>          if (ret != EOK) {
>              DEBUG(SSSDBG_OP_FAILURE,
>                    "Cannot get list of addresses from interface %s\n", iface);
>        /* non critical failure */
>        if (ret == ENOENT) {
>            ret = EOK;

Can you move this block before the debug message or lower the verbosity
of the debug message (ret == ENOENT ? MINOR_FAILURE : OP_FAILURE)
>        }
>    }
>    /* We're done. Just fake an async request completion */
>    goto done;
>
> diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c
> index 689e333d4e68c4a2582894d06b8b7b20c76b9be8..3214e90c063ea9a4cf6f6bc6507bf4e37b7d23a4 100644
> --- a/src/tests/cmocka/test_dyndns.c
> +++ b/src/tests/cmocka/test_dyndns.c
> @@ -247,6 +247,23 @@ void dyndns_test_get_multi_ifaddr(void **state)
>      assert_true(check_leaks_pop(dyndns_test_ctx) == true);
> From b8740437b7a1e6fbe2f7221e5b4c4b44dba2d4d4 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 8 Jul 2015 09:08:03 -0400
> Subject: [PATCH 2/5] DYNDNS: support mult. interfaces for dyndns_iface opt
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2549
>
> src/man/sssd-ad.5.xml            |  7 ++--
>  src/man/sssd-ipa.5.xml           |  7 ++--
>  src/providers/dp_dyndns.c        |  6 ++++
>  src/providers/dp_dyndns.h        |  4 +++
>  src/providers/ldap/sdap_dyndns.c | 72 +++++++++++++++++++++++++++++++++++-----
>  5 files changed, 84 insertions(+), 12 deletions(-)
>
> diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
> index 938a443e027b9bf83c75c240a7d6b2a0876b92c8..4e2aedb57e3d8e1ce5b674e31c6a3d1fe293d4cb 100644
> --- a/src/man/sssd-ad.5.xml
> +++ b/src/man/sssd-ad.5.xml
> @@ -754,15 +754,18 @@ ad_gpo_map_deny = +my_pam_service
>                      <listitem>
>                          <para>
>                              Optional. Applicable only when dyndns_update
>                        is true. Choose the interface whose IP address
>                        is true. Choose the interfaces whose IP addresses
>                        should be used for dynamic DNS updates.
>                    </para>
>                    <para>
>                        NOTE: This option currently supports only one interface.
>                        NOTE: This option supports multiple interfaces.

I think we can just remove this note. If you like, we can say something
like "Choose the interface or a list of interfaces...".
>                      </para>
>                      <para>
>                          Default: Use the IP address of the AD LDAP connection
>                      </para>
>                    <para>
>                        Example: dyndns_iface = em1, vnet1, vnet2
>                    </para>
>                </listitem>
>            </varlistentry>

[...]
> --- a/src/providers/ldap/sdap_dyndns.c
> +++ b/src/providers/ldap/sdap_dyndns.c
> @@ -482,6 +482,65 @@ static void sdap_dyndns_get_addrs_done(struct tevent_req *subreq);
>  static errno_t sdap_dyndns_add_ldap_conn(struct sdap_dyndns_get_addrs_state *state,
>                                           struct sdap_handle *sh);
>
> +static errno_t get_ifaces_addrs(TALLOC_CTX *mem_ctx,
>                            const char *iface,
>                            struct sss_iface_addr **_result)
>
> +{
> struct sss_iface_addr *result_addrs = NULL;
> struct sss_iface_addr *intf_addrs;
> TALLOC_CTX *tmp_ctx;
> char **list_of_intfs;
> int num_of_intfs;
> errno_t ret;
> int i;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    ret = ENOENT;

looks like a typo, please use ENOMEM.
>    goto done;
> }
> ret = split_on_separator(tmp_ctx, iface, ',', true, true, &list_of_intfs,
>                         &num_of_intfs);
> if (ret != EOK) {
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "Parsing names of interfaces failed - %d:[%s].\n",
>          ret, sss_strerror(ret));
>    goto done;
> }
> for (i = 0; i < num_of_intfs; i++) {
>    ret = sss_iface_addr_list_get(tmp_ctx, list_of_intfs[i], &intf_addrs);
>    if (ret == EOK) {
>        if (result_addrs != NULL) {
>            /* If there is already an existing list, head of this existing
>             * list will be considered as parent talloc context for the
>             * new list.
>             */
>            talloc_steal(result_addrs, intf_addrs);
>        }
>        sss_iface_addr_concatenate(&result_addrs, intf_addrs);
>    } else if (ret == ENOENT) {
>        /* non-critical failure */

non-critical failure should not emit OP failure.
>        DEBUG(SSSDBG_OP_FAILURE,
>              "Cannot get interface %s or there are no addresses "
>              "bind to it.\n", list_of_intfs[i]);
>    } else {
>        DEBUG(SSSDBG_OP_FAILURE,
>              "Cannot get list of addresses from interface %s - %d:[%s]\n",
>              list_of_intfs[i], ret, sss_strerror(ret));
>        goto done;
>    }
> }
> ret = EOK;
>
> +done:
> *_result = talloc_steal(mem_ctx, result_addrs);

is it correct to touch the return pointer also if ret != OK ?
> talloc_free(tmp_ctx);
> return ret;
>
> +}
>
> static struct tevent_req *
>  sdap_dyndns_get_addrs_send(TALLOC_CTX *mem_ctx,
>                             struct tevent_context *ev,
> @@ -500,14 +559,11 @@ sdap_dyndns_get_addrs_send(TALLOC_CTX *mem_ctx,
>      }
>  if (iface) {
>    ret = sss_iface_addr_list_get(state, iface, &state->addresses);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE,
>              "Cannot get list of addresses from interface %s\n", iface);
>        /* non critical failure */
>        if (ret == ENOENT) {
>            ret = EOK;
>        }
>    ret = get_ifaces_addrs(state, iface, &state->addresses);
>    if (ret != EOK || state->addresses == NULL) {
>        DEBUG(SSSDBG_MINOR_FAILURE,
>              "get_ifaces_addrs() failed: %d:[%s]\n",
>              ret, sss_strerror(ret));
>    }
>    /* We're done. Just fake an async request completion */
>    goto done;
>
> -- 
> 2.4.3
> From 793de4b7b5762689e011dc220564380cb8094598 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 14 Jul 2015 04:21:34 -0400
> Subject: [PATCH 3/5] DYNDNS: special value '*' for dyndns_iface option
>
> Option dyndns_iface has now special value '*' which implies that IPs
> from add interfaces should be sent during DDNS update.
>
> src/man/sssd-ad.5.xml     |  4 +++-
>  src/man/sssd-ipa.5.xml    |  4 +++-
>  src/providers/dp_dyndns.c | 17 +++++++++++++----
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
> index 4e2aedb57e3d8e1ce5b674e31c6a3d1fe293d4cb..f925790a0cff419dfd4c904fdd45fa4fd76a4a25 100644
> --- a/src/man/sssd-ad.5.xml
> +++ b/src/man/sssd-ad.5.xml
> @@ -755,7 +755,9 @@ ad_gpo_map_deny = +my_pam_service
>                          <para>
>                              Optional. Applicable only when dyndns_update
>                              is true. Choose the interfaces whose IP addresses
>                        should be used for dynamic DNS updates.
>                        should be used for dynamic DNS updates. Special
>                        value <quote>*</quote> implies that IPs from all
>                        interfaces should be used.
>                    </para>
>                    <para>
>                        NOTE: This option supports multiple interfaces.
>
> diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml
> index ca59ffbb005a6f4129362276ce57faa6a9e1af5c..c484204ceb08e34169de5df18b6a01cb09d44ed6 100644
> --- a/src/man/sssd-ipa.5.xml
> +++ b/src/man/sssd-ipa.5.xml
> @@ -167,7 +167,9 @@
>                          <para>
>                              Optional. Applicable only when dyndns_update
>                              is true. Choose the interfaces whose IP addresses
>                        should be used for dynamic DNS updates.
>                        should be used for dynamic DNS updates. Special
>                        value <quote>*</quote> implies that IPs from all
>                        interfaces should be used.
>                    </para>
>                    <para>
>                        NOTE: This option currently supports multiple interfaces.
>
> diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> index dec521054e18e083ba0557dbf9c8bfe918d4575e..a1533387c1fc980713fbc01c25efba2fbaed4a43 100644
> --- a/src/providers/dp_dyndns.c
> +++ b/src/providers/dp_dyndns.c
> @@ -171,6 +171,16 @@ ok_for_dns(struct sockaddr *sa)
>      return true;
>  }
>
> +static bool supported_address_family(sa_family_t sa_family)
> +{
> return sa_family == AF_INET || sa_family == AF_INET6;
>
> +}
>
> +static bool matching_name(const char *ifname, const char *ifname2)
> +{
> return (strcmp("*", ifname) == 0) || (strcasecmp(ifname, ifname2) == 0);

nitpick - can you #define the asterisk as a constant and add a comment
that it's a special value that means all ifaces?
> +}
>
> /* Collect IP addresses associated with an interface */
>  errno_t
>  sss_iface_addr_list_get(TALLOC_CTX *mem_ctx, const char *ifname,
> @@ -200,10 +210,9 @@ sss_iface_addr_list_get(TALLOC_CTX *mem_ctx, const char *ifname,
>          if (!ifa->ifa_addr) continue;
>      /* Add IP addresses to the list */
>    if ((ifa->ifa_addr->sa_family == AF_INET ||
>         ifa->ifa_addr->sa_family == AF_INET6) &&
>         strcasecmp(ifa->ifa_name, ifname) == 0 &&
>         ok_for_dns(ifa->ifa_addr)) {
>    if (supported_address_family(ifa->ifa_addr->sa_family)
>            && matching_name(ifname, ifa->ifa_name)
>            && ok_for_dns(ifa->ifa_addr)) {

thanks, this is more readable.
>          /* Add this address to the IP address list */
>          address = talloc_zero(mem_ctx, struct sss_iface_addr);
>
> -- 
> 2.4.3
> From 24c659db3abd0a9db880ecf8ae040302a4614de6 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 15 Jul 2015 10:58:38 -0400
> Subject: [PATCH 4/5] TESTS: dyndns tests support AAAA addresses

ACK
> From 8680c9df4accf269bdc9d2cb6c3440cbb0dc8405 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 14 Jul 2015 09:56:59 -0400
> Subject: [PATCH 5/5] DYNDNS: support for dualstack
>
> When dyndns_iface option was not used, address of connection to LDAP
> was used. This patch proposes following change:
> Interface containing address of connection is found.
> All A and AAAA addresses of this interface are collected.
> Collected addresses are sent during DDNS update.
> Function sss_iface_addr_add() is removed.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2558
>
> src/providers/dp_dyndns.c        | 150 +++++++++++++++++++++++++++------
>  src/providers/dp_dyndns.h        |   8 +-
>  src/providers/ldap/sdap_dyndns.c |  20 ++---
>  src/tests/cmocka/test_dyndns.c   | 178 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 317 insertions(+), 39 deletions(-)
>
> diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> index a1533387c1fc980713fbc01c25efba2fbaed4a43..4310a7c6c74787eada934e56380375611801245b 100644
> --- a/src/providers/dp_dyndns.c
> +++ b/src/providers/dp_dyndns.c
> @@ -55,31 +55,6 @@ void sss_iface_addr_concatenate(struct sss_iface_addr **list,
>      DLIST_CONCATENATE((*list), list2, struct sss_iface_addr*);
>  }
>
> -struct sss_iface_addr *
> -sss_iface_addr_add(TALLOC_CTX *mem_ctx, struct sss_iface_addr **list,
>               struct sockaddr_storage *ss)
>
> -{
> struct sss_iface_addr *address;
> address = talloc(mem_ctx, struct sss_iface_addr);
> if (address == NULL) {
>    return NULL;
> }
> address->addr = talloc_memdup(address, ss,
>                              sizeof(struct sockaddr_storage));
> if(address->addr == NULL) {
>    talloc_zfree(address);
>    return NULL;
> }
> /* steal old dlist to the new head */
> talloc_steal(address, *list);
> DLIST_ADD(*list, address);
> return address;
>
> -}
>
> errno_t
>  sss_iface_addr_list_as_str_list(TALLOC_CTX *mem_ctx,
>                                  struct sss_iface_addr *ifaddr_list,
> @@ -1252,3 +1227,128 @@ errno_t be_nsupdate_init_timer(struct be_nsupdate_ctx *ctx,
>  return ERR_OK;
>
> }
>
> +static errno_t match_ip(const struct sockaddr *sa,
>                    const struct sockaddr *sb,
>                    bool *_res)

Don't you think it would be simpler to just return bool? For cases where
the address family is totally different, can you just return false?
> +{
> size_t addrsize;
> bool res;
> errno_t ret;
> const void *addr_a;
> const void *addr_b;
> if (sa->sa_family == AF_INET) {
>    addrsize = sizeof(struct in_addr);
>    addr_a = (const void *) &((const struct sockaddr_in *) sa)->sin_addr;
>    addr_b = (const void *) &((const struct sockaddr_in *) sb)->sin_addr;
> } else if (sa->sa_family == AF_INET6) {
>    addrsize = sizeof(struct in6_addr);
>    addr_a = (const void *) &((const struct sockaddr_in6 *) sa)->sin6_addr;
>    addr_b = (const void *) &((const struct sockaddr_in6 *) sb)->sin6_addr;

Hmm, I was surprised I couldn't find any existing function or macro to
compare a sockaddr or a sockaddr_storage except IN6_ARE_ADDR_EQUAL..
> } else {
>    ret = EINVAL;
>    goto done;
> }
> if (sa->sa_family != sb->sa_family) {
>    ret = EOK;
>    res = false;
>    goto done;
> }
> res = memcmp(addr_a, addr_b, addrsize) == 0;
> ret = EOK;
>
> +done:
> if (ret == EOK) {
>    *_res = res;
> }

I think a more idiomatic way is to call this assignment to output
pointer along with setting ret = EOK.
> return ret;
>
> +}
>
> +static errno_t find_iface_by_addr(TALLOC_CTX *mem_ctx,
>                              const struct sockaddr *ss,
>                              const char **_iface_name)
>
> +{
> struct ifaddrs *ifaces = NULL;
> struct ifaddrs *ifa;
> errno_t ret;
> ret = getifaddrs(&ifaces);
> if (ret == -1) {
>    ret = errno;
>    DEBUG(SSSDBG_OP_FAILURE,
>          "Could not read interfaces [%d][%s]\n", ret, sss_strerror(ret));
>    goto done;
> }
> for (ifa = ifaces; ifa != NULL; ifa = ifa->ifa_next) {
>    bool ip_matched;
>    /* Some interfaces don't have an ifa_addr */
>    if (!ifa->ifa_addr) continue;
>    ret = match_ip(ss, ifa->ifa_addr, &ip_matched);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE, "match_ip failed: %d:[%s]\n",
>              ret, sss_strerror(ret));
>        goto done;
>    }
>    if (ip_matched) {
>        const char *iface_name;

Please put a blank line here. Actually, I'm not thrilled to have the EOK
branch in the middle of the function. Normally, you want to outmost
branch to be the success one...here it's ENOENT..
>        iface_name = talloc_strdup(mem_ctx, ifa->ifa_name);
>        if (iface_name == NULL) {
>            ret = ENOMEM;
>        } else {
>            *_iface_name = iface_name;
>            ret = EOK;
>        }
>        goto done;
>    }
> }
> ret = ENOENT;
>
> +done:
> freeifaddrs(ifaces);
> return ret;
>
> +}
>
> +errno_t sss_get_dualstack_addresses(TALLOC_CTX *mem_ctx,
>                                struct sockaddr *ss,
>                                struct sss_iface_addr **_iface_addrs)
>
> +{
> struct sss_iface_addr *iface_addrs;
> const char *iface_name = NULL;
> TALLOC_CTX *tmp_ctx;
> errno_t ret;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> ret = find_iface_by_addr(tmp_ctx, ss, &iface_name);
> if (ret != EOK) {
>    DEBUG(SSSDBG_MINOR_FAILURE, "find_iface_by_addr failed: %d:[%s]\n",
>          ret, sss_strerror(ret));
>    goto done;
> }
> ret = sss_iface_addr_list_get(tmp_ctx, iface_name, &iface_addrs);
> if (ret != EOK) {
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "sss_iface_addr_list_get failed: %d:[%s]\n",
>          ret, sss_strerror(ret));
>    goto done;
> }
>
> +done:
> if (ret == EOK) {
>    *_iface_addrs = talloc_steal(mem_ctx, iface_addrs);

Again, it would be more idiomatic and consistent with the rest of the
code to write:

ret = EOK
    *_iface_addrs = <assign>
done:
> }
> talloc_free(tmp_ctx);
> return ret;
>
> +}

The rest looks OK to me. I've started Coverity scan.

[Pavel Reichl]

Hello Jakub,

thanks  for the comments, I have updated first 4th patches as you 
proposed, but I need some clarification of the last patch, please see 
comments inline:

This relates to the 3rd patch.
> I think we can just remove this note. If you like, we can say 
> something like "Choose the interface or a list of interfaces...".
> >                       </para>
> >                       <para>
> >                           Default: Use the IP address of the AD LDAP connection
> >                       </para>
> >                    <para>
> >                        Example: dyndns_iface = em1, vnet1, vnet2
> >                    </para>
> >                 </listitem>
> >             </varlistentry>

Please note that in attached patches I changed the default.
> > From 8680c9df4accf269bdc9d2cb6c3440cbb0dc8405 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl
> > Date: Tue, 14 Jul 2015 09:56:59 -0400
> > Subject: [PATCH 5/5] DYNDNS: support for dualstack
> >
> > When dyndns_iface option was not used, address of connection to LDAP
> > was used. This patch proposes following change:
> > Interface containing address of connection is found.
> > All A and AAAA addresses of this interface are collected.
> > Collected addresses are sent during DDNS update.
> > Function sss_iface_addr_add() is removed.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2558
> >
> > src/providers/dp_dyndns.c        | 150 +++++++++++++++++++++++++++------
> >   src/providers/dp_dyndns.h        |   8 +-
> >   src/providers/ldap/sdap_dyndns.c |  20 ++---
> >   src/tests/cmocka/test_dyndns.c   | 178 +++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 317 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> > index a1533387c1fc980713fbc01c25efba2fbaed4a43..4310a7c6c74787eada934e56380375611801245b 100644
> > --- a/src/providers/dp_dyndns.c
> > +++ b/src/providers/dp_dyndns.c
> > @@ -55,31 +55,6 @@ void sss_iface_addr_concatenate(struct sss_iface_addr **list,
> >       DLIST_CONCATENATE((*list), list2, struct sss_iface_addr*);
> >   }
> >
> > -struct sss_iface_addr *
> > -sss_iface_addr_add(TALLOC_CTX *mem_ctx, struct sss_iface_addr **list,
> >               struct sockaddr_storage *ss)
> >
> > -{
> > struct sss_iface_addr *address;
> > address = talloc(mem_ctx, struct sss_iface_addr);
> > if (address == NULL) {
> >    return NULL;
> > }
> > address->addr = talloc_memdup(address, ss,
> >                              sizeof(struct sockaddr_storage));
> > if(address->addr == NULL) {
> >    talloc_zfree(address);
> >    return NULL;
> > }
> > /* steal old dlist to the new head */
> > talloc_steal(address, *list);
> > DLIST_ADD(*list, address);
> > return address;
> >
> > -}
> > errno_t
> > sss_iface_addr_list_as_str_list(TALLOC_CTX *mem_ctx,
> >                                 struct sss_iface_addr *ifaddr_list,
> >
> > @@ -1252,3 +1227,128 @@ errno_t be_nsupdate_init_timer(struct be_nsupdate_ctx *ctx,
> >   return ERR_OK;
> >
> > }
> >
> > +static errno_t match_ip(const struct sockaddr *sa,
> >                    const struct sockaddr *sb,
> >                    bool *_res)
>
> Don't you think it would be simpler to just return bool? For cases where

Yes, it would be simpler, but I prefer to know that address is from 
family that is not supported by the function. But I can change it if you 
still prefer that.
> the address family is totally different, can you just return false?

I actually think this is how function works (at least for address 
families known to function)
> > +{
> > size_t addrsize;
> > bool res;
> > errno_t ret;
> > const void *addr_a;
> > const void *addr_b;
> > if (sa->sa_family == AF_INET) {
> >    addrsize = sizeof(struct in_addr);
> >    addr_a = (const void *) &((const struct sockaddr_in *) sa)->sin_addr;
> >    addr_b = (const void *) &((const struct sockaddr_in *) sb)->sin_addr;
> > } else if (sa->sa_family == AF_INET6) {
> >    addrsize = sizeof(struct in6_addr);
> >    addr_a = (const void *) &((const struct sockaddr_in6 *) sa)->sin6_addr;
> >    addr_b = (const void *) &((const struct sockaddr_in6 *) sb)->sin6_addr;
>
> Hmm, I was surprised I couldn't find any existing function or macro to
> compare a sockaddr or a sockaddr_storage except IN6_ARE_ADDR_EQUAL..
> > } else {
> >    ret = EINVAL;
> >    goto done;

unsupported address family
> > }
> > if (sa->sa_family != sb->sa_family) {
> >    ret = EOK;
> >    res = false;
> >    goto done;
> > }

For different families I return false.
> > res = memcmp(addr_a, addr_b, addrsize) == 0;
> > ret = EOK;
> >
> > +done:
> > if (ret == EOK) {
> >    *_res = res;
> > }
>
> I think a more idiomatic way is to call this assignment to output
> pointer along with setting ret = EOK.

Yes, it is. The reason I don't follow this idiomatic way here is the 
fact that "ret = EOK" is written at 2 places in the function and I don't 
want to duplicate code. But it would be quite minimal change here. Still 
I personally prefer it the way it is now. You decide :-).
> > return ret;
> >
> > +}
> >
> > +static errno_t find_iface_by_addr(TALLOC_CTX *mem_ctx,
> >                              const struct sockaddr *ss,
> >                              const char **_iface_name)
> >
> > +{
> > struct ifaddrs *ifaces = NULL;
> > struct ifaddrs *ifa;
> > errno_t ret;
> > ret = getifaddrs(&ifaces);
> > if (ret == -1) {
> >    ret = errno;
> >    DEBUG(SSSDBG_OP_FAILURE,
> >          "Could not read interfaces [%d][%s]\n", ret, sss_strerror(ret));
> >    goto done;
> > }
> > for (ifa = ifaces; ifa != NULL; ifa = ifa->ifa_next) {
> >    bool ip_matched;
> >    /* Some interfaces don't have an ifa_addr */
> >    if (!ifa->ifa_addr) continue;
> >    ret = match_ip(ss, ifa->ifa_addr, &ip_matched);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_OP_FAILURE, "match_ip failed: %d:[%s]\n",
> >              ret, sss_strerror(ret));
> >        goto done;
> >    }
> >    if (ip_matched) {
> >        const char *iface_name;
>
> Please put a blank line here. Actually, I'm not thrilled to have the EOK
> branch in the middle of the function. Normally, you want to outmost
> branch to be the success one...here it's ENOENT..

I'm sorry I fail to see any more elegant way how to rewrite the function.
> >        iface_name = talloc_strdup(mem_ctx, ifa->ifa_name);
> >        if (iface_name == NULL) {
> >            ret = ENOMEM;
> >        } else {
> >            *_iface_name = iface_name;
> >            ret = EOK;

I can add break here
> >        }
> >        goto done;
> >    }
> > }
> > ret = ENOENT;

and test here "iface_name != NUL => EOK", if you think this will improve 
the readability of the function.
> > +done:
> > freeifaddrs(ifaces);
> > return ret;
> >
> > +}
> >
> > +errno_t sss_get_dualstack_addresses(TALLOC_CTX *mem_ctx,
> >                                struct sockaddr *ss,
> >                                struct sss_iface_addr **_iface_addrs)
> >
> > +{
> > struct sss_iface_addr *iface_addrs;
> > const char *iface_name = NULL;
> > TALLOC_CTX *tmp_ctx;
> > errno_t ret;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> > }
> > ret = find_iface_by_addr(tmp_ctx, ss, &iface_name);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_MINOR_FAILURE, "find_iface_by_addr failed: %d:[%s]\n",
> >          ret, sss_strerror(ret));
> >    goto done;
> > }
> > ret = sss_iface_addr_list_get(tmp_ctx, iface_name, &iface_addrs);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_MINOR_FAILURE,
> >          "sss_iface_addr_list_get failed: %d:[%s]\n",
> >          ret, sss_strerror(ret));
> >    goto done;
> > }
> >
> > +done:
> > if (ret == EOK) {
> >    *_iface_addrs = talloc_steal(mem_ctx, iface_addrs);
>
> Again, it would be more idiomatic and consistent with the rest of the
> code to write:
>  ret = EOK
>  *_iface_addrs = <assign>
>
> done:

OK, I'll fix it.
> > }
> > talloc_free(tmp_ctx);
> > return ret;
> >
> > +}
>
> The rest looks OK to me. I've started Coverity scan.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Jul 22, 2015 at 07:54:43PM +0200, Pavel Reichl wrote:
> Hello Jakub,
>
> thanks  for the comments, I have updated first 4th patches as you proposed,
> but I need some clarification of the last patch, please see comments inline:

The first patches are mostly good now, I only have two minor comments:
> This relates to the 3rd patch.
> > I think we can just remove this note. If you like, we can say something
> > like "Choose the interface or a list of interfaces...".
> > >                      </para>
> > >                      <para>
> > >                          Default: Use the IP address of the AD LDAP connection
> > >                      </para>
> > >                    <para>
> > >                        Example: dyndns_iface = em1, vnet1, vnet2
> > >                    </para>
> > >                </listitem>
> > >            </varlistentry>
>
> Please note that in attached patches I changed the default.

OK
> > > From 8680c9df4accf269bdc9d2cb6c3440cbb0dc8405 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl
> > > Date: Tue, 14 Jul 2015 09:56:59 -0400
> > > Subject: [PATCH 5/5] DYNDNS: support for dualstack
> > >
> > > When dyndns_iface option was not used, address of connection to LDAP
> > > was used. This patch proposes following change:
> > > Interface containing address of connection is found.
> > > All A and AAAA addresses of this interface are collected.
> > > Collected addresses are sent during DDNS update.
> > > Function sss_iface_addr_add() is removed.
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2558
> > >
> > > src/providers/dp_dyndns.c        | 150 +++++++++++++++++++++++++++------
> > >  src/providers/dp_dyndns.h        |   8 +-
> > >  src/providers/ldap/sdap_dyndns.c |  20 ++---
> > >  src/tests/cmocka/test_dyndns.c   | 178 +++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 317 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> > > index a1533387c1fc980713fbc01c25efba2fbaed4a43..4310a7c6c74787eada934e56380375611801245b 100644
> > > --- a/src/providers/dp_dyndns.c
> > > +++ b/src/providers/dp_dyndns.c
> > > @@ -55,31 +55,6 @@ void sss_iface_addr_concatenate(struct sss_iface_addr **list,
> > >      DLIST_CONCATENATE((*list), list2, struct sss_iface_addr*);
> > >  }
> > > -struct sss_iface_addr *
> > > -sss_iface_addr_add(TALLOC_CTX *mem_ctx, struct sss_iface_addr **list,
> > >               struct sockaddr_storage *ss)
> > >
> > > -{
> > > struct sss_iface_addr *address;
> > > address = talloc(mem_ctx, struct sss_iface_addr);
> > > if (address == NULL) {
> > >    return NULL;
> > > }
> > > address->addr = talloc_memdup(address, ss,
> > >                              sizeof(struct sockaddr_storage));
> > > if(address->addr == NULL) {
> > >    talloc_zfree(address);
> > >    return NULL;
> > > }
> > > /* steal old dlist to the new head */
> > > talloc_steal(address, *list);
> > > DLIST_ADD(*list, address);
> > > return address;
> > >
> > > -}
> > >
> > > errno_t
> > >  sss_iface_addr_list_as_str_list(TALLOC_CTX *mem_ctx,
> > >                                  struct sss_iface_addr *ifaddr_list,
> > > @@ -1252,3 +1227,128 @@ errno_t be_nsupdate_init_timer(struct be_nsupdate_ctx *ctx,
> > >      return ERR_OK;
> > >  }
> > >
> > > +static errno_t match_ip(const struct sockaddr *sa,
> > >                    const struct sockaddr *sb,
> > >                    bool *_res)
> >
> > Don't you think it would be simpler to just return bool? For cases where
>
> Yes, it would be simpler, but I prefer to know that address is from family
> that is not supported by the function. But I can change it if you still
> prefer that.
> > the address family is totally different, can you just return false?
>
> I actually think this is how function works (at least for address families
> known to function)
> > > +{
> > > size_t addrsize;
> > > bool res;
> > > errno_t ret;
> > > const void *addr_a;
> > > const void *addr_b;
> > > if (sa->sa_family == AF_INET) {
> > >    addrsize = sizeof(struct in_addr);
> > >    addr_a = (const void *) &((const struct sockaddr_in *) sa)->sin_addr;
> > >    addr_b = (const void *) &((const struct sockaddr_in *) sb)->sin_addr;
> > > } else if (sa->sa_family == AF_INET6) {
> > >    addrsize = sizeof(struct in6_addr);
> > >    addr_a = (const void *) &((const struct sockaddr_in6 *) sa)->sin6_addr;
> > >    addr_b = (const void *) &((const struct sockaddr_in6 *) sb)->sin6_addr;
> >
> > Hmm, I was surprised I couldn't find any existing function or macro to
> > compare a sockaddr or a sockaddr_storage except IN6_ARE_ADDR_EQUAL..
> > > } else {
> > >    ret = EINVAL;
> > >    goto done;
>
> unsupported address family

I was thinking just return false here and skip that IP, just like for
different families and skip the invalid address...

btw what other address can there be on an interface?
> > > }
> > > if (sa->sa_family != sb->sa_family) {
> > >    ret = EOK;
> > >    res = false;
> > >    goto done;
> > > }
>
> For different families I return false.
> > > res = memcmp(addr_a, addr_b, addrsize) == 0;
> > > ret = EOK;
> > >
> > > +done:
> > > if (ret == EOK) {
> > >    *_res = res;
> > > }
> >
> > I think a more idiomatic way is to call this assignment to output
> > pointer along with setting ret = EOK.
>
> Yes, it is. The reason I don't follow this idiomatic way here is the fact
> that "ret = EOK" is written at 2 places in the function and I don't want to
> duplicate code. But it would be quite minimal change here. Still I
> personally prefer it the way it is now. You decide :-).
> > > return ret;
> > >
> > > +}
> > >
> > > +static errno_t find_iface_by_addr(TALLOC_CTX *mem_ctx,
> > >                              const struct sockaddr *ss,
> > >                              const char **_iface_name)
> > >
> > > +{
> > > struct ifaddrs *ifaces = NULL;
> > > struct ifaddrs *ifa;
> > > errno_t ret;
> > > ret = getifaddrs(&ifaces);
> > > if (ret == -1) {
> > >    ret = errno;
> > >    DEBUG(SSSDBG_OP_FAILURE,
> > >          "Could not read interfaces [%d][%s]\n", ret, sss_strerror(ret));
> > >    goto done;
> > > }
> > > for (ifa = ifaces; ifa != NULL; ifa = ifa->ifa_next) {
> > >    bool ip_matched;
> > >    /* Some interfaces don't have an ifa_addr */
> > >    if (!ifa->ifa_addr) continue;
> > >    ret = match_ip(ss, ifa->ifa_addr, &ip_matched);
> > >    if (ret != EOK) {
> > >        DEBUG(SSSDBG_OP_FAILURE, "match_ip failed: %d:[%s]\n",
> > >              ret, sss_strerror(ret));
> > >        goto done;
> > >    }
> > >    if (ip_matched) {
> > >        const char *iface_name;
> >
> > Please put a blank line here. Actually, I'm not thrilled to have the EOK
> > branch in the middle of the function. Normally, you want to outmost
> > branch to be the success one...here it's ENOENT..
>
> I'm sorry I fail to see any more elegant way how to rewrite the function.

OK, please at least add a blank line.
> > >        iface_name = talloc_strdup(mem_ctx, ifa->ifa_name);
> > >        if (iface_name == NULL) {
> > >            ret = ENOMEM;
> > >        } else {
> > >            *_iface_name = iface_name;
> > >            ret = EOK;
>
> I can add break here
> > >        }
> > >        goto done;
> > >    }
> > > }
> > > ret = ENOENT;
>
> and test here "iface_name != NUL => EOK", if you think this will improve the
> readability of the function.
> > > +done:
> > > freeifaddrs(ifaces);
> > > return ret;
> > >
> > > +}
> > >
> > > +errno_t sss_get_dualstack_addresses(TALLOC_CTX *mem_ctx,
> > >                                struct sockaddr *ss,
> > >                                struct sss_iface_addr **_iface_addrs)
> > >
> > > +{
> > > struct sss_iface_addr *iface_addrs;
> > > const char *iface_name = NULL;
> > > TALLOC_CTX *tmp_ctx;
> > > errno_t ret;
> > > tmp_ctx = talloc_new(NULL);
> > > if (tmp_ctx == NULL) {
> > >    ret = ENOMEM;
> > >    goto done;
> > > }
> > > ret = find_iface_by_addr(tmp_ctx, ss, &iface_name);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_MINOR_FAILURE, "find_iface_by_addr failed: %d:[%s]\n",
> > >          ret, sss_strerror(ret));
> > >    goto done;
> > > }
> > > ret = sss_iface_addr_list_get(tmp_ctx, iface_name, &iface_addrs);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > >          "sss_iface_addr_list_get failed: %d:[%s]\n",
> > >          ret, sss_strerror(ret));
> > >    goto done;
> > > }
> > >
> > > +done:
> > > if (ret == EOK) {
> > >    *_iface_addrs = talloc_steal(mem_ctx, iface_addrs);
> >
> > Again, it would be more idiomatic and consistent with the rest of the
> > code to write:
> > ret = EOK
> > *_iface_addrs = <assign>
> >
> > done:
>
> OK, I'll fix it.
> > > }
> > > talloc_free(tmp_ctx);
> > > return ret;
> > >
> > > +}
> >
> > The rest looks OK to me. I've started Coverity scan.

Coverity didn't find any issues btw.
> From 166f56fb9d749c5ee72d61d8e2e5f1c9c6dafb5b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 8 Jul 2015 09:01:24 -0400
> Subject: [PATCH 1/5] DYNDNS: sss_iface_addr_list_get return ENOENT

ACK
> From 18353c25b99674a524e4a459a710e091d6a68cc1 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 8 Jul 2015 09:08:03 -0400
> Subject: [PATCH 2/5] DYNDNS: support mult. interfaces for dyndns_iface opt

[...]
> @@ -482,6 +482,65 @@ static void sdap_dyndns_get_addrs_done(struct tevent_req *subreq);
>  static errno_t sdap_dyndns_add_ldap_conn(struct sdap_dyndns_get_addrs_state *state,
>                                           struct sdap_handle *sh);
>
> +static errno_t get_ifaces_addrs(TALLOC_CTX *mem_ctx,
>                            const char *iface,
>                            struct sss_iface_addr **_result)
>
> +{
> struct sss_iface_addr *result_addrs = NULL;
> struct sss_iface_addr *intf_addrs;
> TALLOC_CTX *tmp_ctx;
> char **list_of_intfs;
> int num_of_intfs;
> errno_t ret;
> int i;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    ret = ENONEN;

This is a typo fixed in the next patch. Please squash the fix here to
make each patch compile on its own.
>    goto done;
> }
> ret = split_on_separator(tmp_ctx, iface, ',', true, true, &list_of_intfs,
> From 1def75b238ac8c9da397a14e074e30a50eaf4606 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 14 Jul 2015 04:21:34 -0400
> Subject: [PATCH 3/5] DYNDNS: special value '*' for dyndns_iface option
>
> Option dyndns_iface has now special value '*' which implies that IPs
> from add interfaces should be sent during DDNS update.

[...]
>  return true;
>
> }
>
> +static bool supported_address_family(sa_family_t sa_family)
> +{
> return sa_family == AF_INET || sa_family == AF_INET6;
>
> +}
>
> +/* MASK represents special value for matching all interfaces */
> +#define MASK "*"

I would prefer to move the #define to the top of the file.
> +static bool matching_name(const char *ifname, const char *ifname2)
> +{
> return (strcmp(MASK, ifname) == 0) || (strcasecmp(ifname, ifname2) == 0);
>
> +}
> From 60f14b58fce74518964d51478299cf1ca1aff5b9 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Wed, 15 Jul 2015 10:58:38 -0400
> Subject: [PATCH 4/5] TESTS: dyndns tests support AAAA addresses

ACK

[Pavel Reichl]

On 07/23/2015 11:42 AM, Jakub Hrozek wrote:
> +static errno_t match_ip(const struct sockaddr *sa,
>                    const struct sockaddr *sb,
>                    bool *_res)
> > > Don't you think it would be simpler to just return bool? For cases where
> >
> > Yes, it would be simpler, but I prefer to know that address is from family
> > that is not supported by the function. But I can change it if you still
> > prefer that.
> > > the address family is totally different, can you just return false?
> >
> > I actually think this is how function works (at least for address families
> > known to function)
> > > > +{
> > > > size_t addrsize;
> > > > bool res;
> > > > errno_t ret;
> > > > const void *addr_a;
> > > > const void *addr_b;
> > > > if (sa->sa_family == AF_INET) {
> > > >    addrsize = sizeof(struct in_addr);
> > > >    addr_a = (const void *) &((const struct sockaddr_in *) sa)->sin_addr;
> > > >    addr_b = (const void *) &((const struct sockaddr_in *) sb)->sin_addr;
> > > > } else if (sa->sa_family == AF_INET6) {
> > > >    addrsize = sizeof(struct in6_addr);
> > > >    addr_a = (const void *) &((const struct sockaddr_in6 *) sa)->sin6_addr;
> > > >    addr_b = (const void *) &((const struct sockaddr_in6 *) sb)->sin6_addr;
> > >
> > > Hmm, I was surprised I couldn't find any existing function or macro to
> > > compare a sockaddr or a sockaddr_storage except IN6_ARE_ADDR_EQUAL..
> > > > } else {
> > > >    ret = EINVAL;
> > > >    goto done;
> >
> > unsupported address family
>
> I was thinking just return false here and skip that IP, just like for
> different families and skip the invalid address...

OK, I'll do it as you propose.
> btw what other address can there be on an interface?

I honestly don't know, that's why I tried to be careful about it.

[Jakub Hrozek]

On Thu, Jul 23, 2015 at 12:28:33PM +0200, Pavel Reichl wrote:
> On 07/23/2015 11:42 AM, Jakub Hrozek wrote:
> > +static errno_t match_ip(const struct sockaddr *sa,
> >                    const struct sockaddr *sb,
> >                    bool *_res)
> > > > Don't you think it would be simpler to just return bool? For cases where
> > >
> > > Yes, it would be simpler, but I prefer to know that address is from family
> > > that is not supported by the function. But I can change it if you still
> > > prefer that.
> > > > the address family is totally different, can you just return false?
> > >
> > > I actually think this is how function works (at least for address families
> > > known to function)
> > > > > +{
> > > > > size_t addrsize;
> > > > > bool res;
> > > > > errno_t ret;
> > > > > const void *addr_a;
> > > > > const void *addr_b;
> > > > > if (sa->sa_family == AF_INET) {
> > > > >    addrsize = sizeof(struct in_addr);
> > > > >    addr_a = (const void *) &((const struct sockaddr_in *) sa)->sin_addr;
> > > > >    addr_b = (const void *) &((const struct sockaddr_in *) sb)->sin_addr;
> > > > > } else if (sa->sa_family == AF_INET6) {
> > > > >    addrsize = sizeof(struct in6_addr);
> > > > >    addr_a = (const void *) &((const struct sockaddr_in6 *) sa)->sin6_addr;
> > > > >    addr_b = (const void *) &((const struct sockaddr_in6 *) sb)->sin6_addr;
> > > >
> > > > Hmm, I was surprised I couldn't find any existing function or macro to
> > > > compare a sockaddr or a sockaddr_storage except IN6_ARE_ADDR_EQUAL..
> > > > > } else {
> > > > >    ret = EINVAL;
> > > > >    goto done;
> > >
> > > unsupported address family
> >
> > I was thinking just return false here and skip that IP, just like for
> > different families and skip the invalid address...
>
> OK, I'll do it as you propose.
> > btw what other address can there be on an interface?
>
> I honestly don't know, that's why I tried to be careful about it.

Well, do you see any risk if there was an invalid address and we skipped
it?

Is there a test that would try the nothing matched scenario? If yes,
then I would prefer to simplify the code, otherwise keep your more
paranoid less readable version.

[Pavel Reichl]

I hope attached patch set addresses all concerns, thanks!

[Jakub Hrozek]

On Thu, Jul 23, 2015 at 01:47:19PM +0200, Pavel Reichl wrote:
> I hope attached patch set addresses all concerns, thanks!

My issues were addressed and the code seems to work. Coverity didn't
find anything either.

ACK

[Jakub Hrozek]

On Thu, Jul 23, 2015 at 09:54:49PM +0200, Jakub Hrozek wrote:
> On Thu, Jul 23, 2015 at 01:47:19PM +0200, Pavel Reichl wrote:
> > I hope attached patch set addresses all concerns, thanks!
>
> My issues were addressed and the code seems to work. Coverity didn't
> find anything either.
>
> ACK

* master:
    * b0a8ed519554f8896e35812e0759862c33f157fe
    * 1112e84494bcfd0f658e073d25f15ed877d047aa
    * 0a26e92fb2a4dd9704a0578f90241997e2aed269
    * 038b9ba28a618e3e553803da632116a040b94034
    * aa3fd6fde3888c0e333cad852ae5b4f671d55f58