Hello,
please see attached patches.
1st patch adds return value ENOENT to sss_iface_addr_list_get() so I can provide more concrete debug message for missing interface or if interface is not suitable (missing IP address)
2nd patch: * I introduced new public function sss_iface_addr_concatenate(), I'm aware that this function is probably not reusable but I needed to work around that 'struct sss_iface_addr' in defined in source file only. * I had troubles with correctly handling creating talloc hiearchy of IPs of different interfaces. I decided to use first address of first found interface as a parent talloc context for other interfaces. I attached talloc report output to illustrate this.
- full talloc report on 'struct sdap_dyndns_get_addrs_state' (total 16 bytes in 1 blocks)
- full talloc report on 'struct sdap_dyndns_get_addrs_state' (total 376 bytes in 19 blocks)
blocks (ref 0) 0xbc0650struct sss_iface_addr contains 360 bytes in 18
- struct sss_iface_addr contains 120 bytes in 6 blocks (ref 0) 0xbecee0
- struct sss_iface_addr contains 80 bytes in 4 blocks (ref 0) 0xbeb920
blocks (ref 0) 0xbd03f0struct sss_iface_addr contains 40 bytes in 2
1 blocks (ref 0) 0xbd0470../src/providers/dp_dyndns.c:219 contains 16 bytes in
blocks (ref 0) 0xbeb9a0../src/providers/dp_dyndns.c:219 contains 16 bytes in 1
- ../src/providers/dp_dyndns.c:219 contains 16 bytes in 1 blocks (ref 0) 0xbecf60
- struct sss_iface_addr contains 120 bytes in 6 blocks (ref 0) 0xbd0640
- struct sss_iface_addr contains 80 bytes in 4 blocks (ref 0) 0xbd19a0
blocks (ref 0) 0xbcfb00struct sss_iface_addr contains 40 bytes in 2
1 blocks (ref 0) 0xbed300../src/providers/dp_dyndns.c:219 contains 16 bytes in
blocks (ref 0) 0xbd1a20../src/providers/dp_dyndns.c:219 contains 16 bytes in 1
- ../src/providers/dp_dyndns.c:219 contains 16 bytes in 1 blocks (ref 0) 0xbd06c0
- struct sss_iface_addr contains 80 bytes in 4 blocks (ref 0) 0xbd0eb0
- struct sss_iface_addr contains 40 bytes in 2 blocks (ref 0) 0xbd1900
blocks (ref 0) 0xbec4f0../src/providers/dp_dyndns.c:219 contains 16 bytes in 1
- ../src/providers/dp_dyndns.c:219 contains 16 bytes in 1 blocks (ref 0) 0xbeca10
- ../src/providers/dp_dyndns.c:219 contains 16 bytes in 1 blocks (ref 0) 0xbe6ae0
* I was thinking whether it would be a good idea to handle the case when processing of interfaces provided in dyndns_iface yields no address at all by continuing to detect DYNDNS address from LDAP connection?
Thanks!
On 07/09/2015 11:15 AM, Pavel Reichl wrote:
Hello,
please see attached patches.
1st patch adds return value ENOENT to sss_iface_addr_list_get() so I can provide more concrete debug message for missing interface or if interface is not suitable (missing IP address)
2nd patch:
- I introduced new public function sss_iface_addr_concatenate(), I'm
aware that this function is probably not reusable but I needed to work around that 'struct sss_iface_addr' in defined in source file only.
- I had troubles with correctly handling creating talloc hiearchy of
IPs of different interfaces. I decided to use first address of first found interface as a parent talloc context for other interfaces. I attached talloc report output to illustrate this.
- full talloc report on 'struct sdap_dyndns_get_addrs_state' (total 16 bytes in 1 blocks)
- full talloc report on 'struct sdap_dyndns_get_addrs_state' (total 376 bytes in 19 blocks)
- struct sss_iface_addr contains 360 bytes in 18 blocks (ref 0) 0xbc0650
blocks (ref 0) 0xbecee0struct sss_iface_addr contains 120 bytes in 6
4 blocks (ref 0) 0xbeb920struct sss_iface_addr contains 80 bytes in
2 blocks (ref 0) 0xbd03f0struct sss_iface_addr contains 40 bytes in
16 bytes in 1 blocks (ref 0) 0xbd0470../src/providers/dp_dyndns.c:219 contains
bytes in 1 blocks (ref 0) 0xbeb9a0../src/providers/dp_dyndns.c:219 contains 16
in 1 blocks (ref 0) 0xbecf60../src/providers/dp_dyndns.c:219 contains 16 bytes
blocks (ref 0) 0xbd0640struct sss_iface_addr contains 120 bytes in 6
4 blocks (ref 0) 0xbd19a0struct sss_iface_addr contains 80 bytes in
2 blocks (ref 0) 0xbcfb00struct sss_iface_addr contains 40 bytes in
16 bytes in 1 blocks (ref 0) 0xbed300../src/providers/dp_dyndns.c:219 contains
bytes in 1 blocks (ref 0) 0xbd1a20../src/providers/dp_dyndns.c:219 contains 16
in 1 blocks (ref 0) 0xbd06c0../src/providers/dp_dyndns.c:219 contains 16 bytes
blocks (ref 0) 0xbd0eb0struct sss_iface_addr contains 80 bytes in 4
2 blocks (ref 0) 0xbd1900struct sss_iface_addr contains 40 bytes in
bytes in 1 blocks (ref 0) 0xbec4f0../src/providers/dp_dyndns.c:219 contains 16
in 1 blocks (ref 0) 0xbeca10../src/providers/dp_dyndns.c:219 contains 16 bytes
1 blocks (ref 0) 0xbe6ae0../src/providers/dp_dyndns.c:219 contains 16 bytes in
- I was thinking whether it would be a good idea to handle the case
when processing of interfaces provided in dyndns_iface yields no address at all by continuing to detect DYNDNS address from LDAP connection?
Thanks!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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...
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.
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.
We want to support dualstack in IPA for long time. Several users wanted dualstack, even multihomed support.
Martin2
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?
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.
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
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!
On 07/15/2015 07:40 PM, Pavel Reichl wrote:
[snip]
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 to help of Petr Spacek I was able to fix my AD configuration so my tests in AD and IPA were both successful!
Thanks!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 16/07/15 11:01, Pavel Reichl wrote:
On 07/15/2015 07:40 PM, Pavel Reichl wrote:
[snip]
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 to help of Petr Spacek I was able to fix my AD configuration so my tests in AD and IPA were both successful!
Thanks!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Works for me, Pavel is God.
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 preichl@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 preichl@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 preichl@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 preichl@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 preichl@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.
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:
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.
From 8680c9df4accf269bdc9d2cb6c3440cbb0dc8405 Mon Sep 17 00:00:00 2001 From: Pavel Reichlpreichl@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
- }
- 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.
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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 Reichlpreichl@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@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@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@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@redhat.com Date: Wed, 15 Jul 2015 10:58:38 -0400 Subject: [PATCH 4/5] TESTS: dyndns tests support AAAA addresses
ACK
On 07/23/2015 11:42 AM, Jakub Hrozek wrote:
+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...
OK, I'll do it as you propose.
btw what other address can there be on an interface?
I honestly don't know, that's why I tried to be careful about it.
On Thu, Jul 23, 2015 at 12:28:33PM +0200, Pavel Reichl wrote:
On 07/23/2015 11:42 AM, Jakub Hrozek wrote:
+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...
OK, I'll do it as you propose.
btw what other address can there be on an interface?
I honestly don't know, that's why I tried to be careful about it.
Well, do you see any risk if there was an invalid address and we skipped it?
Is there a test that would try the nothing matched scenario? If yes, then I would prefer to simplify the code, otherwise keep your more paranoid less readable version.
On Thu, Jul 23, 2015 at 09:54:49PM +0200, Jakub Hrozek wrote:
On Thu, Jul 23, 2015 at 01:47:19PM +0200, Pavel Reichl wrote:
I hope attached patch set addresses all concerns, thanks!
My issues were addressed and the code seems to work. Coverity didn't find anything either.
ACK
* master: * b0a8ed519554f8896e35812e0759862c33f157fe * 1112e84494bcfd0f658e073d25f15ed877d047aa * 0a26e92fb2a4dd9704a0578f90241997e2aed269 * 038b9ba28a618e3e553803da632116a040b94034 * aa3fd6fde3888c0e333cad852ae5b4f671d55f58
sssd-devel@lists.fedorahosted.org