[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