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

Jakub Hrozek jhrozek at redhat.com
Thu Jul 23 09:42:50 UTC 2015


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

ACK



More information about the sssd-devel mailing list