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 > >>>>sssd-devel@lists.fedorahosted.org > >>>>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 > >>sssd-devel@lists.fedorahosted.org > >>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 > >sssd-devel@lists.fedorahosted.org > >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 <prei...@redhat.com> > 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 <prei...@redhat.com> > 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 <prei...@redhat.com> > 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 <prei...@redhat.com> > 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 <prei...@redhat.com> > 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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
