On Tue, Aug 11, 2015 at 01:26:47PM +0200, Pavel Reichl wrote:
On 07/28/2015 01:06 PM, Jakub Hrozek wrote:
>On Tue, Jul 28, 2015 at 01:02:39PM +0200, Pavel Reichl wrote:
>>
>>On 07/24/2015 09:46 PM, Jakub Hrozek wrote:
>>>>On 24 Jul 2015, at 19:44, Pavel Reichl <preichl(a)redhat.com> wrote:
>>>>
>>>>
>>>>On 07/24/2015 07:41 PM, Pavel Reichl wrote:
>>>>>Hello,
>>>>>
>>>>>please see attached patch set.
>>>>>
>>>>>Thanks!
>>>>>
>>>>>
>>>>>_______________________________________________
>>>>>sssd-devel mailing list
>>>>>
>>>>>sssd-devel(a)lists.fedorahosted.org
>>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>Hmm...I should have sent this to the thread "DYNDNS: Add a new
option dyndns_server", sorry.
>>>I think it's fine, rather than re-sending the patchset, please just send
a note to that thread.
>>>
>>>As a first thing to fix, please come up with a better title for patch #2 :-)
>>OK, no problem I can do that. I consider this 2nd patch to be ephemera that
>>will be definitely squashed into 1st patch and thus won't be part of git
>>history, so the caption doesn't matter at all. I could squash it myself but
>>I thought it will be easier for reviewer (probably you) to see your patch
>>unchanged and my proposed changes as two separate patches. Please correct me
>>if I'm wrong.
>>
>>Anyway I think I can postpone the change until more comments come, right?
>Sure.
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello, I have rebased patch set, changed name of second patch and commit
message of 0004-DYNDNS-remove-zone-command.patch/
Sorry for delay.
From 4ac6071a397b315957f0257bd0c9467ca48fcec0 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Sun, 6 Jul 2014 22:53:27 +0200
Subject: [PATCH 1/9] DYNDNS: Add a new option dyndns_server
Can you review this patch?
From 618230028bac79daed2447d5ea428bf62ee20c26 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 23 Jul 2015 04:40:03 -0400
Subject: [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
https://fedorahosted.org/sssd/ticket/2495
ACK, but can you add some explanation to the commit message?
From a92cc49ec5f3d2848ec4a013bd882bafc82c7cb3 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 23 Jul 2015 04:50:38 -0400
Subject: [PATCH 3/9] DYNDNS: remove redundant talloc_steal()
String 'update_msg' was already allocated on mem_ctx, so, there is no
need to steal it.
ACK
From 822fb54e81a71138271fa4f273aa9d6c8fea9f01 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 23 Jul 2015 05:30:34 -0400
Subject: [PATCH 4/9] DYNDNS: remove zone command
Remove zone command from message to nsupsate. This command is generally
used to hint nsupdate. In correctly configured environment such
information should be obtained via DNS.
If DNS does not provide necessary information we give other hints.
For more details see:
https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Resolves:
https://fedorahosted.org/sssd/ticket/2495
ACK
From ee1c2db6ed6047d56158cad9819dca8074e54f71 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 23 Jul 2015 09:52:47 -0400
Subject: [PATCH 5/9] DYNDNS: rename field of sdap_dyndns_update_state
Rename 'use_server_with_nsupdate' to more general name 'fallback_mode'.
Resolves:
https://fedorahosted.org/sssd/ticket/2495
ACK
From 4807c5d5f641f561d02f3671da36227da181358c Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 23 Jul 2015 10:51:50 -0400
Subject: [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
Resolves:
https://fedorahosted.org/sssd/ticket/2195
[...]
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index ae3f913ee392a6513f75aab497e7f2d784784748..efa6d73dcf66cb685cf3f331b38e75b5435aead1
100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -394,11 +394,16 @@ nsupdate_msg_create_common(TALLOC_CTX *mem_ctx, const char *realm,
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) return NULL;
+ if (realm != NULL) {
#ifdef HAVE_NSUPDATE_REALM
realm_directive = talloc_asprintf(tmp_ctx, "realm %s\n", realm);
#else
realm_directive = talloc_asprintf(tmp_ctx, "\n");
#endif
+ } else {
+ realm_directive = talloc_asprintf(tmp_ctx, "\n");
+ }
+
Please move this block into a function..
From cc90ec5bedfcbe094c44457c9339035096f50509 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Fri, 24 Jul 2015 08:24:48 -0400
Subject: [PATCH 7/9] DYNDNS: remove code duplication
Move copy pasted code for converting sockaddr_storage to string into
function.
Resolves:
https://fedorahosted.org/sssd/ticket/2495
ACK. I was thinking about adding a mem_ctx to the function but we use
the string as an argument for asprintf so the stack-allocated buffer is
probably better.
From d0a0e516723161504900f05d0504288c73c29000 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Fri, 24 Jul 2015 10:41:42 -0400
Subject: [PATCH 8/9] TESTS: UT for sss_iface_addr_list_as_str_list()
ACK
From 02e090379b5ea50578b7f6451e89df0dd418762e Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Fri, 24 Jul 2015 13:25:56 -0400
Subject: [PATCH 9/9] DYNDNS: fix nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family
processed IP address belongs to.
Please reword the commit message, the "fix" part sounds scary. Please
also add the explanation why the separate sends are preferable.
[...]
--- a/src/tests/cmocka/test_dyndns.c
+++ b/src/tests/cmocka/test_dyndns.c
@@ -328,6 +328,266 @@ void dyndns_test_addr_list_as_str_list(void **state)
assert_true(check_leaks_pop(dyndns_test_ctx) == true);
}
+void dyndns_test_create_fwd_msg(void **state)
+{
+ errno_t ret;
+ char *msg;
+ struct sss_iface_addr *addrlist;
+ int i;
+
+ check_leaks_push(dyndns_test_ctx);
+
+ /* getifaddrs is called twice in sss_get_dualstack_addresses() */
+ for (i = 0; i < 2; i++) {
+ will_return_getifaddrs("eth0", "192.168.0.2", AF_INET);
+ will_return_getifaddrs("eth1", "192.168.0.1", AF_INET);
+ will_return_getifaddrs("eth0", "2001:cdba::555", AF_INET6);
+ will_return_getifaddrs("eth1", "2001:cdba::444", AF_INET6);
+ will_return_getifaddrs(NULL, NULL, 0); /* sentinel */
+ }
+
+ struct sockaddr_in sin;
+ memset (&sin, 0, sizeof (sin));
I don't like the spaces after calling a function, style-wise.
+ sin.sin_family = AF_INET;
+ sin.sin_addr.s_addr = inet_addr ("192.168.0.2");
+ ret = sss_get_dualstack_addresses(dyndns_test_ctx,
+ (struct sockaddr *) &sin,
+ &addrlist);