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<preichl(a)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
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 <preichl(a)redhat.com>
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 <preichl(a)redhat.com>
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 <preichl(a)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.
[...]
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 <preichl(a)redhat.com>
Date: Wed, 15 Jul 2015 10:58:38 -0400
Subject: [PATCH 4/5] TESTS: dyndns tests support AAAA addresses
ACK