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<[email protected]> > >>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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > 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 <[email protected]> > Date: Wed, 15 Jul 2015 10:58:38 -0400 > Subject: [PATCH 4/5] TESTS: dyndns tests support AAAA addresses ACK _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
