On 07/24/2015 07:41 PM, Pavel Reichl wrote:
Hello,
please see attached patch set.
Thanks!
sssd-devel mailing list sssd-devel@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.
On 24 Jul 2015, at 19:44, Pavel Reichl preichl@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@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 :-)
On 07/24/2015 09:46 PM, Jakub Hrozek wrote:
On 24 Jul 2015, at 19:44, Pavel Reichl preichl@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@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?
Thanks!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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@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.
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@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@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@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.
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@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@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@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@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@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
ACK, but can you add some explanation to the commit message?
From a92cc49ec5f3d2848ec4a013bd882bafc82c7cb3 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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@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
ACK
From ee1c2db6ed6047d56158cad9819dca8074e54f71 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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'.
ACK
From 4807c5d5f641f561d02f3671da36227da181358c Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
[...]
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@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.
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@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@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);
[snip]
From 4ac6071a397b315957f0257bd0c9467ca48fcec0 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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? +++ b/src/man/sssd-ad.5.xml @@ -812,6 +812,26 @@ ad_gpo_map_deny = +my_pam_service </listitem> </varlistentry>
<varlistentry>
<term>dyndns_server (string)</term>
<listitem>
<para>
The DNS server to use when performing a DNS
update. In most setups, it's recommended
to leave
this option unset.
</para>
<para>
Setting this option makes sense for
environments
where the DNS server is different from
the identity
server.
</para>
<para>
Default: None (let nsupdate choose the
server)
</para>
</listitem>
</varlistentry>
One empty line too many?
<xi:include
xmlns:xi="http://www.w3.org/2001/XInclude" href="include/override_homedir.xml" /> <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="include/homedir_substring.xml" />
ACK, I'm not running CI, I would prefer to run in on whole patch set.
I was thinking whether we should amend the man page to state explicitly that dyndns_server is not used in the first attempt to perform DNS udpate, but only in the subsequent (failover) attempt that contains such hints. However, I suppose that such change should be part of the patches that actually change this behavior...or we could be silent about this...?
From 618230028bac79daed2447d5ea428bf62ee20c26 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
ACK, but can you add some explanation to the commit message?
done [snip]
From 4807c5d5f641f561d02f3671da36227da181358c Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
[...]
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..
done [snip]
From 02e090379b5ea50578b7f6451e89df0dd418762e Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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.
Done. [snip]
- struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
I don't like the spaces after calling a function, style-wise.
done
- 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);
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 08/13/2015 02:12 PM, Pavel Reichl wrote:
Sorry, this time with correct patches!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I updated the second patch - [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
I added some minor man page changes.
Thanks!
On Thu, Aug 13, 2015 at 05:31:07PM +0200, Pavel Reichl wrote:
On 08/13/2015 02:12 PM, Pavel Reichl wrote:
Sorry, this time with correct patches!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I updated the second patch - [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
I added some minor man page changes.
Thanks!
From 7d786a6a102f274a685b9e84ee7fe9cdf2872590 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 6 Jul 2014 22:53:27 +0200 Subject: [PATCH 1/9] DYNDNS: Add a new option dyndns_server
The changes you requested were done to this patch. Thanks for catching those.
From 8a880df3631d08f195868d7de47e685a91533b09 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
ACK
From c865cd09cc418f3ddda93d0e1e73c541becb7309 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 04:50:38 -0400 Subject: [PATCH 3/9] DYNDNS: remove redundant talloc_steal()
ACK
From 0ccb4fc0902fc29e2b452dbef4d0e6f8bb685ba4 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 05:30:34 -0400 Subject: [PATCH 4/9] DYNDNS: remove zone command
ACK
From 0bf7d58da64c40c52b29a3d89e2e40308898b976 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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'.
ACK
From 748d3e0f584a2469cdb7333a579a924a54c0b7dc Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 08:24:48 -0400 Subject: [PATCH 7/9] DYNDNS: remove code duplication
ACK
From ee8d4ef5bdc0b843a8a069bbda58507b4bc9b390 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 d49e79e45579b65ed482364be853292343abed9e Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 9/9] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Nack, the test doesn't work: ./dyndns-tests [==========] Running 15 test(s). [ RUN ] dyndns_test_get_ifaddr [ OK ] dyndns_test_get_ifaddr [ RUN ] dyndns_test_get_multi_ifaddr [ OK ] dyndns_test_get_multi_ifaddr [ RUN ] dyndns_test_get_ifaddr_enoent [ OK ] dyndns_test_get_ifaddr_enoent [ RUN ] dyndns_test_addr_list_as_str_list [ OK ] dyndns_test_addr_list_as_str_list [ RUN ] dyndns_test_ok ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header [ OK ] dyndns_test_ok [ RUN ] dyndns_test_error (Fri Aug 14 16:09:41:326570 2015) [sssd] [child_sig_handler] (0x0020): child [6436] failed with status [1]. [ OK ] dyndns_test_error [ RUN ] dyndns_test_timeout (Fri Aug 14 16:09:43:329799 2015) [sssd] [nsupdate_child_timeout] (0x0020): Timeout reached for dynamic DNS update (Fri Aug 14 16:09:43:330390 2015) [sssd] [child_sig_handler] (0x0020): child [6437] was terminated by signal [9]. [ OK ] dyndns_test_timeout [ RUN ] dyndns_test_interval [ OK ] dyndns_test_interval [ RUN ] dyndns_test_dualstack [ OK ] dyndns_test_dualstack [ RUN ] dyndns_test_dualstack_multiple_addresses [ OK ] dyndns_test_dualstack_multiple_addresses [ RUN ] dyndns_test_dualstack_no_iface [ OK ] dyndns_test_dualstack_no_iface [ RUN ] dyndns_test_create_fwd_msg Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg [ RUN ] dyndns_test_create_fwd_msg_mult Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_mult [ RUN ] dyndns_test_create_fwd_msg_A Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_A [ RUN ] dyndns_test_create_fwd_msg_AAAA Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_AAAA [==========] 15 test(s) run. [ PASSED ] 11 test(s).
But I'm going to push the acked patches..
On Fri, Aug 14, 2015 at 04:12:05PM +0200, Jakub Hrozek wrote:
But I'm going to push the acked patches..
The first patches were pushed: 6fd5306145d98ea3bab7f32aa66475f610f388ce b42bf6c0c01db08208fb81d8295a2909d307284a 76604931b11594394a05df10f8370a1b8bb3e54b 4f2a07c422fa357ef6651bca8c48b8005280fa1d e4d6e9ccac14044d6bcd5a0dce7f45fdfab6bf3d 7c3cc1ee2914bc7b38a992c1af254fc76af5a1ad 8145ab51b05aa86b2f1a21b49383f55e50b0a2e3
Please respin the last one.
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
On Thu, Aug 13, 2015 at 05:31:07PM +0200, Pavel Reichl wrote:
On 08/13/2015 02:12 PM, Pavel Reichl wrote:
Sorry, this time with correct patches!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I updated the second patch - [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
I added some minor man page changes.
Thanks! From 7d786a6a102f274a685b9e84ee7fe9cdf2872590 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 6 Jul 2014 22:53:27 +0200 Subject: [PATCH 1/9] DYNDNS: Add a new option dyndns_server
The changes you requested were done to this patch. Thanks for catching those.
From 8a880df3631d08f195868d7de47e685a91533b09 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
ACK
From c865cd09cc418f3ddda93d0e1e73c541becb7309 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 04:50:38 -0400 Subject: [PATCH 3/9] DYNDNS: remove redundant talloc_steal()
ACK
From 0ccb4fc0902fc29e2b452dbef4d0e6f8bb685ba4 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 05:30:34 -0400 Subject: [PATCH 4/9] DYNDNS: remove zone command
ACK
From 0bf7d58da64c40c52b29a3d89e2e40308898b976 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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'.
ACK
Missing [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
From 748d3e0f584a2469cdb7333a579a924a54c0b7dc Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 08:24:48 -0400 Subject: [PATCH 7/9] DYNDNS: remove code duplication
ACK
From ee8d4ef5bdc0b843a8a069bbda58507b4bc9b390 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 d49e79e45579b65ed482364be853292343abed9e Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 9/9] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Nack, the test doesn't work: ./dyndns-tests [==========] Running 15 test(s). [ RUN ] dyndns_test_get_ifaddr [ OK ] dyndns_test_get_ifaddr [ RUN ] dyndns_test_get_multi_ifaddr [ OK ] dyndns_test_get_multi_ifaddr [ RUN ] dyndns_test_get_ifaddr_enoent [ OK ] dyndns_test_get_ifaddr_enoent [ RUN ] dyndns_test_addr_list_as_str_list [ OK ] dyndns_test_addr_list_as_str_list [ RUN ] dyndns_test_ok ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header [ OK ] dyndns_test_ok [ RUN ] dyndns_test_error (Fri Aug 14 16:09:41:326570 2015) [sssd] [child_sig_handler] (0x0020): child [6436] failed with status [1]. [ OK ] dyndns_test_error [ RUN ] dyndns_test_timeout (Fri Aug 14 16:09:43:329799 2015) [sssd] [nsupdate_child_timeout] (0x0020): Timeout reached for dynamic DNS update (Fri Aug 14 16:09:43:330390 2015) [sssd] [child_sig_handler] (0x0020): child [6437] was terminated by signal [9]. [ OK ] dyndns_test_timeout [ RUN ] dyndns_test_interval [ OK ] dyndns_test_interval [ RUN ] dyndns_test_dualstack [ OK ] dyndns_test_dualstack [ RUN ] dyndns_test_dualstack_multiple_addresses [ OK ] dyndns_test_dualstack_multiple_addresses [ RUN ] dyndns_test_dualstack_no_iface [ OK ] dyndns_test_dualstack_no_iface [ RUN ] dyndns_test_create_fwd_msg Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg [ RUN ] dyndns_test_create_fwd_msg_mult Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_mult [ RUN ] dyndns_test_create_fwd_msg_A Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_A [ RUN ] dyndns_test_create_fwd_msg_AAAA Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_AAAA [==========] 15 test(s) run. [ PASSED ] 11 test(s).
But I'm going to push the acked patches.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
On Thu, Aug 13, 2015 at 05:31:07PM +0200, Pavel Reichl wrote:
On 08/13/2015 02:12 PM, Pavel Reichl wrote:
Sorry, this time with correct patches!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I updated the second patch - [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
I added some minor man page changes.
Thanks! From 7d786a6a102f274a685b9e84ee7fe9cdf2872590 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 6 Jul 2014 22:53:27 +0200 Subject: [PATCH 1/9] DYNDNS: Add a new option dyndns_server
The changes you requested were done to this patch. Thanks for catching those.
From 8a880df3631d08f195868d7de47e685a91533b09 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
ACK
From c865cd09cc418f3ddda93d0e1e73c541becb7309 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 04:50:38 -0400 Subject: [PATCH 3/9] DYNDNS: remove redundant talloc_steal()
ACK
From 0ccb4fc0902fc29e2b452dbef4d0e6f8bb685ba4 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 05:30:34 -0400 Subject: [PATCH 4/9] DYNDNS: remove zone command
ACK
From 0bf7d58da64c40c52b29a3d89e2e40308898b976 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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'.
ACK
Missing [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
From 748d3e0f584a2469cdb7333a579a924a54c0b7dc Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 08:24:48 -0400 Subject: [PATCH 7/9] DYNDNS: remove code duplication
ACK
From ee8d4ef5bdc0b843a8a069bbda58507b4bc9b390 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 d49e79e45579b65ed482364be853292343abed9e Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 9/9] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Nack, the test doesn't work: ./dyndns-tests [==========] Running 15 test(s). [ RUN ] dyndns_test_get_ifaddr [ OK ] dyndns_test_get_ifaddr [ RUN ] dyndns_test_get_multi_ifaddr [ OK ] dyndns_test_get_multi_ifaddr [ RUN ] dyndns_test_get_ifaddr_enoent [ OK ] dyndns_test_get_ifaddr_enoent [ RUN ] dyndns_test_addr_list_as_str_list [ OK ] dyndns_test_addr_list_as_str_list [ RUN ] dyndns_test_ok ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header [ OK ] dyndns_test_ok [ RUN ] dyndns_test_error (Fri Aug 14 16:09:41:326570 2015) [sssd] [child_sig_handler] (0x0020): child [6436] failed with status [1]. [ OK ] dyndns_test_error [ RUN ] dyndns_test_timeout (Fri Aug 14 16:09:43:329799 2015) [sssd] [nsupdate_child_timeout] (0x0020): Timeout reached for dynamic DNS update (Fri Aug 14 16:09:43:330390 2015) [sssd] [child_sig_handler] (0x0020): child [6437] was terminated by signal [9]. [ OK ] dyndns_test_timeout [ RUN ] dyndns_test_interval [ OK ] dyndns_test_interval [ RUN ] dyndns_test_dualstack [ OK ] dyndns_test_dualstack [ RUN ] dyndns_test_dualstack_multiple_addresses [ OK ] dyndns_test_dualstack_multiple_addresses [ RUN ] dyndns_test_dualstack_no_iface [ OK ] dyndns_test_dualstack_no_iface [ RUN ] dyndns_test_create_fwd_msg Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg [ RUN ] dyndns_test_create_fwd_msg_mult Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_mult [ RUN ] dyndns_test_create_fwd_msg_A Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_A [ RUN ] dyndns_test_create_fwd_msg_AAAA Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_AAAA [==========] 15 test(s) run. [ PASSED ] 11 test(s).
But I'm going to push the acked patches.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Bump
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
On Thu, Aug 13, 2015 at 05:31:07PM +0200, Pavel Reichl wrote:
On 08/13/2015 02:12 PM, Pavel Reichl wrote:
Sorry, this time with correct patches!
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I updated the second patch - [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
I added some minor man page changes.
Thanks! From 7d786a6a102f274a685b9e84ee7fe9cdf2872590 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 6 Jul 2014 22:53:27 +0200 Subject: [PATCH 1/9] DYNDNS: Add a new option dyndns_server
The changes you requested were done to this patch. Thanks for catching those.
From 8a880df3631d08f195868d7de47e685a91533b09 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
ACK
From c865cd09cc418f3ddda93d0e1e73c541becb7309 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 04:50:38 -0400 Subject: [PATCH 3/9] DYNDNS: remove redundant talloc_steal()
ACK
From 0ccb4fc0902fc29e2b452dbef4d0e6f8bb685ba4 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 05:30:34 -0400 Subject: [PATCH 4/9] DYNDNS: remove zone command
ACK
From 0bf7d58da64c40c52b29a3d89e2e40308898b976 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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'.
ACK
Missing [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
From 748d3e0f584a2469cdb7333a579a924a54c0b7dc Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 08:24:48 -0400 Subject: [PATCH 7/9] DYNDNS: remove code duplication
ACK
From ee8d4ef5bdc0b843a8a069bbda58507b4bc9b390 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 d49e79e45579b65ed482364be853292343abed9e Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 9/9] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Nack, the test doesn't work: ./dyndns-tests [==========] Running 15 test(s). [ RUN ] dyndns_test_get_ifaddr [ OK ] dyndns_test_get_ifaddr [ RUN ] dyndns_test_get_multi_ifaddr [ OK ] dyndns_test_get_multi_ifaddr [ RUN ] dyndns_test_get_ifaddr_enoent [ OK ] dyndns_test_get_ifaddr_enoent [ RUN ] dyndns_test_addr_list_as_str_list [ OK ] dyndns_test_addr_list_as_str_list [ RUN ] dyndns_test_ok ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header [ OK ] dyndns_test_ok [ RUN ] dyndns_test_error (Fri Aug 14 16:09:41:326570 2015) [sssd] [child_sig_handler] (0x0020): child [6436] failed with status [1]. [ OK ] dyndns_test_error [ RUN ] dyndns_test_timeout (Fri Aug 14 16:09:43:329799 2015) [sssd] [nsupdate_child_timeout] (0x0020): Timeout reached for dynamic DNS update (Fri Aug 14 16:09:43:330390 2015) [sssd] [child_sig_handler] (0x0020): child [6437] was terminated by signal [9]. [ OK ] dyndns_test_timeout [ RUN ] dyndns_test_interval [ OK ] dyndns_test_interval [ RUN ] dyndns_test_dualstack [ OK ] dyndns_test_dualstack [ RUN ] dyndns_test_dualstack_multiple_addresses [ OK ] dyndns_test_dualstack_multiple_addresses [ RUN ] dyndns_test_dualstack_no_iface [ OK ] dyndns_test_dualstack_no_iface [ RUN ] dyndns_test_create_fwd_msg Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg [ RUN ] dyndns_test_create_fwd_msg_mult Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_mult [ RUN ] dyndns_test_create_fwd_msg_A Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_A [ RUN ] dyndns_test_create_fwd_msg_AAAA Could not run the test - check test fixtures [ ERROR ] dyndns_test_create_fwd_msg_AAAA [==========] 15 test(s) run. [ PASSED ] 11 test(s).
But I'm going to push the acked patches.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
Hi,
DYNDNS: use realm and server commands only as fallback Resolves: https://fedorahosted.org/sssd/ticket/2195
You have a wrong ticket number here, I think. The patches look good otherwise.
On 09/21/2015 10:52 AM, Pavel Březina wrote:
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
Hi,
DYNDNS: use realm and server commands only as fallback Resolves: https://fedorahosted.org/sssd/ticket/2195
You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote:
On 09/21/2015 10:52 AM, Pavel Březina wrote:
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
Hi,
DYNDNS: use realm and server commands only as fallback
You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote:
On 09/21/2015 10:52 AM, Pavel Březina wrote:
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
Hi,
DYNDNS: use realm and server commands only as fallback Resolves: https://fedorahosted.org/sssd/ticket/2195
You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote:
On 09/21/2015 10:52 AM, Pavel Březina wrote:
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
Hi,
DYNDNS: use realm and server commands only as fallback
You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote:
On 09/21/2015 10:52 AM, Pavel Březina wrote:
On 08/28/2015 11:05 AM, Pavel Reichl wrote:
On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
I must have skipped it while respinning the patch set, sorry.
I attached this missing patch and test that was failing because of it.
Hi,
DYNDNS: use realm and server commands only as fallback Resolves: https://fedorahosted.org/sssd/ticket/2195
You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote:
On 09/21/2015 10:52 AM, Pavel Březina wrote:
On 08/28/2015 11:05 AM, Pavel Reichl wrote: >On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
>Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >server commands only as fallback > >I must have skipped it while respinning the patch set, sorry. > >I attached this missing patch and test that was failing because of it.
Hi,
> DYNDNS: use realm and server commands only as fallback > > Resolves: > https://fedorahosted.org/sssd/ticket/2195
You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
From 82c9372e5c0c38e83a87867cfc046ab1610791ae Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
[...]
- update_msg = talloc_asprintf_append(update_msg, "send\n");
- /* AAAA addresses first */
Should this say AAAA addresses next?
- /* Remove existing entries as needed */ if (remove_af & DYNDNS_REMOVE_AAAA) {
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote:
On 09/21/2015 10:52 AM, Pavel Březina wrote: > On 08/28/2015 11:05 AM, Pavel Reichl wrote: >> On 08/14/2015 04:12 PM, Jakub Hrozek wrote:
>> Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >> server commands only as fallback >> >> I must have skipped it while respinning the patch set, sorry. >> >> I attached this missing patch and test that was failing because of it. > > Hi, > >> DYNDNS: use realm and server commands only as fallback >> >> Resolves: >> https://fedorahosted.org/sssd/ticket/2195 > > You have a wrong ticket number here, I think. The patches look good otherwise.
Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
Petr, do you agree? Thanks!
From 82c9372e5c0c38e83a87867cfc046ab1610791ae Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
[...]
- update_msg = talloc_asprintf_append(update_msg, "send\n");
- /* AAAA addresses first */
Should this say AAAA addresses next?
I suppose so, probably copy & paste mistake.
- /* Remove existing entries as needed */ if (remove_af & DYNDNS_REMOVE_AAAA) {
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Oct 05, 2015 at 01:35:04PM +0200, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: > > >On 09/21/2015 10:52 AM, Pavel Březina wrote: >>On 08/28/2015 11:05 AM, Pavel Reichl wrote: >>>On 08/14/2015 04:12 PM, Jakub Hrozek wrote: > >>>Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >>>server commands only as fallback >>> >>>I must have skipped it while respinning the patch set, sorry. >>> >>>I attached this missing patch and test that was failing because of it. >> >>Hi, >> >>> DYNDNS: use realm and server commands only as fallback >>> >>> Resolves: >>> https://fedorahosted.org/sssd/ticket/2195 >> >>You have a wrong ticket number here, I think. The patches look good otherwise. > >Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
Petr, do you agree? Thanks!
If RHEL-6 and later contain the nsupdate version with realm, then I would prefer to remove this code..it wouldn't be tested anyway (we don't have separately built unit tests for this condition..)
But that can be done in a follow-up patch.
On (05/10/15 13:43), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 01:35:04PM +0200, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote: >On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: >> >> >>On 09/21/2015 10:52 AM, Pavel Březina wrote: >>>On 08/28/2015 11:05 AM, Pavel Reichl wrote: >>>>On 08/14/2015 04:12 PM, Jakub Hrozek wrote: >> >>>>Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >>>>server commands only as fallback >>>> >>>>I must have skipped it while respinning the patch set, sorry. >>>> >>>>I attached this missing patch and test that was failing because of it. >>> >>>Hi, >>> >>>> DYNDNS: use realm and server commands only as fallback >>>> >>>> Resolves: >>>> https://fedorahosted.org/sssd/ticket/2195 >>> >>>You have a wrong ticket number here, I think. The patches look good otherwise. >> >>Yes, thanks for noticing! Updated patchset attached. > >https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is >closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
Petr, do you agree? Thanks!
If RHEL-6 and later contain the nsupdate version with realm, then I would prefer to remove this code..it wouldn't be tested anyway (we don't have separately built unit tests for this condition..)
But that can be done in a follow-up patch.
I think there are distributions which does not have it. IIRC altlinux. I would need to check.
LS
On Mon, Oct 05, 2015 at 02:12:15PM +0200, Lukas Slebodnik wrote:
On (05/10/15 13:43), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 01:35:04PM +0200, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote: > > >On 09/21/2015 05:47 PM, Jakub Hrozek wrote: >>On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: >>> >>> >>>On 09/21/2015 10:52 AM, Pavel Březina wrote: >>>>On 08/28/2015 11:05 AM, Pavel Reichl wrote: >>>>>On 08/14/2015 04:12 PM, Jakub Hrozek wrote: >>> >>>>>Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >>>>>server commands only as fallback >>>>> >>>>>I must have skipped it while respinning the patch set, sorry. >>>>> >>>>>I attached this missing patch and test that was failing because of it. >>>> >>>>Hi, >>>> >>>>> DYNDNS: use realm and server commands only as fallback >>>>> >>>>> Resolves: >>>>> https://fedorahosted.org/sssd/ticket/2195 >>>> >>>>You have a wrong ticket number here, I think. The patches look good otherwise. >>> >>>Yes, thanks for noticing! Updated patchset attached. >> >>https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is >>closed, did you mean another one? > >We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
Petr, do you agree? Thanks!
If RHEL-6 and later contain the nsupdate version with realm, then I would prefer to remove this code..it wouldn't be tested anyway (we don't have separately built unit tests for this condition..)
But that can be done in a follow-up patch.
I think there are distributions which does not have it. IIRC altlinux. I would need to check.
Do they really ship so obsolete package versions or are they a minimalistic environment that strips features?
Does it make sense to support distributions like this with SSSD master? I mean, there's no way for us to test such codepaths anyway..for the specific realm example, it's not a big deal, but for others it might..
On (05/10/15 20:58), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 02:12:15PM +0200, Lukas Slebodnik wrote:
On (05/10/15 13:43), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 01:35:04PM +0200, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote: >On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote: >> >> >>On 09/21/2015 05:47 PM, Jakub Hrozek wrote: >>>On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: >>>> >>>> >>>>On 09/21/2015 10:52 AM, Pavel Březina wrote: >>>>>On 08/28/2015 11:05 AM, Pavel Reichl wrote: >>>>>>On 08/14/2015 04:12 PM, Jakub Hrozek wrote: >>>> >>>>>>Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >>>>>>server commands only as fallback >>>>>> >>>>>>I must have skipped it while respinning the patch set, sorry. >>>>>> >>>>>>I attached this missing patch and test that was failing because of it. >>>>> >>>>>Hi, >>>>> >>>>>> DYNDNS: use realm and server commands only as fallback >>>>>> >>>>>> Resolves: >>>>>> https://fedorahosted.org/sssd/ticket/2195 >>>>> >>>>>You have a wrong ticket number here, I think. The patches look good otherwise. >>>> >>>>Yes, thanks for noticing! Updated patchset attached. >>> >>>https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is >>>closed, did you mean another one? >> >>We cleared that off-list - patch was accidentally omitted from the original patch set. > >Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
Petr, do you agree? Thanks!
If RHEL-6 and later contain the nsupdate version with realm, then I would prefer to remove this code..it wouldn't be tested anyway (we don't have separately built unit tests for this condition..)
But that can be done in a follow-up patch.
I think there are distributions which does not have it. IIRC altlinux. I would need to check.
Do they really ship so obsolete package versions or are they a minimalistic environment that strips features?
Does it make sense to support distributions like this with SSSD master? I mean, there's no way for us to test such codepaths anyway..for the specific realm example, it's not a big deal, but for others it might..
What are consequences if the realm is not supported and we remove ifdef?
LS
On Tue, Oct 06, 2015 at 07:39:24AM +0200, Lukas Slebodnik wrote:
On (05/10/15 20:58), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 02:12:15PM +0200, Lukas Slebodnik wrote:
On (05/10/15 13:43), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 01:35:04PM +0200, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote: > > >On 09/30/2015 03:43 PM, Jakub Hrozek wrote: >>On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote: >>> >>> >>>On 09/21/2015 05:47 PM, Jakub Hrozek wrote: >>>>On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: >>>>> >>>>> >>>>>On 09/21/2015 10:52 AM, Pavel Březina wrote: >>>>>>On 08/28/2015 11:05 AM, Pavel Reichl wrote: >>>>>>>On 08/14/2015 04:12 PM, Jakub Hrozek wrote: >>>>> >>>>>>>Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >>>>>>>server commands only as fallback >>>>>>> >>>>>>>I must have skipped it while respinning the patch set, sorry. >>>>>>> >>>>>>>I attached this missing patch and test that was failing because of it. >>>>>> >>>>>>Hi, >>>>>> >>>>>>> DYNDNS: use realm and server commands only as fallback >>>>>>> >>>>>>> Resolves: >>>>>>> https://fedorahosted.org/sssd/ticket/2195 >>>>>> >>>>>>You have a wrong ticket number here, I think. The patches look good otherwise. >>>>> >>>>>Yes, thanks for noticing! Updated patchset attached. >>>> >>>>https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is >>>>closed, did you mean another one? >>> >>>We cleared that off-list - patch was accidentally omitted from the original patch set. >> >>Can you rebase these patches, please? > >Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
> From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 >From: Pavel Reichl preichl@redhat.com >Date: Thu, 23 Jul 2015 10:51:50 -0400 >Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback > >Resolves: >https://fedorahosted.org/sssd/ticket/2495 >---
[...]
>@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, > return update_msg; > } > >+static char * >+nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) >+{ >+ if (realm != NULL) { >+ return talloc_asprintf(mem_ctx, "realm %s\n", realm); >+ } >+ return talloc_asprintf(mem_ctx, "\n"); >+} >+#else >+static char * >+nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) >+{ >+ return talloc_asprintf(mem_ctx, "\n"); >+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
Petr, do you agree? Thanks!
If RHEL-6 and later contain the nsupdate version with realm, then I would prefer to remove this code..it wouldn't be tested anyway (we don't have separately built unit tests for this condition..)
But that can be done in a follow-up patch.
I think there are distributions which does not have it. IIRC altlinux. I would need to check.
Do they really ship so obsolete package versions or are they a minimalistic environment that strips features?
Does it make sense to support distributions like this with SSSD master? I mean, there's no way for us to test such codepaths anyway..for the specific realm example, it's not a big deal, but for others it might..
What are consequences if the realm is not supported and we remove ifdef?
We'll try to execute nsupdate with a parameter it doesn't know, which will blow up, nsupdate won't work.
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
On 09/30/2015 03:43 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:40:34AM +0200, Pavel Reichl wrote:
On 09/21/2015 05:47 PM, Jakub Hrozek wrote:
On Mon, Sep 21, 2015 at 01:03:13PM +0200, Pavel Reichl wrote: > > > On 09/21/2015 10:52 AM, Pavel Březina wrote: >> On 08/28/2015 11:05 AM, Pavel Reichl wrote: >>> On 08/14/2015 04:12 PM, Jakub Hrozek wrote: > >>> Test failed because of missing patch [PATCH 6/9] DYNDNS: use realm and >>> server commands only as fallback >>> >>> I must have skipped it while respinning the patch set, sorry. >>> >>> I attached this missing patch and test that was failing because of it. >> >> Hi, >> >>> DYNDNS: use realm and server commands only as fallback >>> >>> Resolves: >>> https://fedorahosted.org/sssd/ticket/2195 >> >> You have a wrong ticket number here, I think. The patches look good >> otherwise. > > Yes, thanks for noticing! Updated patchset attached.
https://fedorahosted.org/sssd/ticket/2495 that the ticket reference is closed, did you mean another one?
We cleared that off-list - patch was accidentally omitted from the original patch set.
Can you rebase these patches, please?
Sure, rebased patches attached.
The patches seem to work well for both IPA and AD, CI also passes: http://sssd-ci.duckdns.org/logs/job/29/29/summary.html
See two questions inline:
From b59a854b5a57cfbedd79a5d3b42555c65d4a3c1f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 23 Jul 2015 10:51:50 -0400 Subject: [PATCH 1/2] DYNDNS: use realm and server commands only as fallback
Resolves: https://fedorahosted.org/sssd/ticket/2495
[...]
@@ -375,6 +375,23 @@ static char *nsupdate_msg_add_ptr(char *update_msg, return update_msg; }
+static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- if (realm != NULL) {
return talloc_asprintf(mem_ctx, "realm %s\n", realm);
- }
- return talloc_asprintf(mem_ctx, "\n");
+} +#else +static char * +nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) +{
- return talloc_asprintf(mem_ctx, "\n");
+}
Wouldn't this be more compact and readable:
Sure, I'll do that.
static char * nsupdate_msg_add_realm_cmd(TALLOC_CTX *mem_ctx, const char *realm) { #ifdef HAVE_NSUPDATE_REALM if (realm != NULL) { return talloc_asprintf(mem_ctx, "realm %s\n", realm); } #endif return talloc_asprintf(mem_ctx, "\n"); }
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Petr, do you agree? Thanks!
From 82c9372e5c0c38e83a87867cfc046ab1610791ae Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
[...]
- update_msg = talloc_asprintf_append(update_msg, "send\n");
- /* AAAA addresses first */
Should this say AAAA addresses next?
I suppose so, probably copy & paste mistake.
- /* Remove existing entries as needed */ if (remove_af & DYNDNS_REMOVE_AAAA) {
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
On Mon, Oct 05, 2015 at 02:14:37PM +0200, Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
ACK
On Mon, Oct 05, 2015 at 08:54:06PM +0200, Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 02:14:37PM +0200, Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
ACK
* 12a1c64105ff56b39e197264fec2d9aba6b84185 * a741d0c4345dceb27dfbb20b81fd858a514b47cd
On (05/10/15 14:14), Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
From bdd7d705a218d3e2b450ddcefe65b57cd414765d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Resolves: https://fedorahosted.org/sssd/ticket/2495
src/providers/dp_dyndns.c | 56 ++++++--- src/tests/cmocka/test_dyndns.c | 280 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 20 deletions(-)
diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c index 815acf6963916bbfa5069c700360fa2fb2ca4e1e..43df0e1b58793e2efe55ea1317fc1f7ca8cc579f 100644 --- a/src/tests/cmocka/test_dyndns.c +++ b/src/tests/cmocka/test_dyndns.c @@ -355,6 +355,266 @@ void dyndns_test_addr_list_as_str_list(void **state) assert_true(check_leaks_pop(dyndns_test_ctx) == true); }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
Can somebody explain how is this chage rellated to the grouping A addresses into one transaction?
Unrelated changes should not be part of another ticket. It just obfluscate output of git blame.
sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); ret = sss_get_dualstack_addresses(dyndns_test_ctx,
@@ -436,7 +696,7 @@ void dyndns_test_dualstack_multiple_addresses(void **state) }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
^ Moreover this change was half done. There is still extra space.
LS
On 10/06/2015 07:45 AM, Lukas Slebodnik wrote:
On (05/10/15 14:14), Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
From bdd7d705a218d3e2b450ddcefe65b57cd414765d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Resolves: https://fedorahosted.org/sssd/ticket/2495
src/providers/dp_dyndns.c | 56 ++++++--- src/tests/cmocka/test_dyndns.c | 280 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 20 deletions(-)
diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c index 815acf6963916bbfa5069c700360fa2fb2ca4e1e..43df0e1b58793e2efe55ea1317fc1f7ca8cc579f 100644 --- a/src/tests/cmocka/test_dyndns.c +++ b/src/tests/cmocka/test_dyndns.c @@ -355,6 +355,266 @@ void dyndns_test_addr_list_as_str_list(void **state) assert_true(check_leaks_pop(dyndns_test_ctx) == true); }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
Can somebody explain how is this chage rellated to the grouping A addresses into one transaction?
It's not related. It's just a little code style change being in the same function as other changes in this patch.
I'm aware that you recently became passionate about such irrelevant changes, however other developers seems not to mind. Might be you can try to formulate such a rule for our code style guidelines and if agreed to we can adhere to it?
Unrelated changes should not be part of another ticket. It just obfluscate output of git blame.
sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); ret = sss_get_dualstack_addresses(dyndns_test_ctx,
@@ -436,7 +696,7 @@ void dyndns_test_dualstack_multiple_addresses(void **state) }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
^
Moreover this change was half done. There is still extra space.
Yes, you are right. I'm just not sure it's worth to send another patch to fix such little problem.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (06/10/15 10:03), Pavel Reichl wrote:
On 10/06/2015 07:45 AM, Lukas Slebodnik wrote:
On (05/10/15 14:14), Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
Additionally, what versions of nsupdate do /not/ have the realm command? If they are very old, can we just drop the support for them? (I haven't checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
From bdd7d705a218d3e2b450ddcefe65b57cd414765d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Resolves: https://fedorahosted.org/sssd/ticket/2495
src/providers/dp_dyndns.c | 56 ++++++--- src/tests/cmocka/test_dyndns.c | 280 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 20 deletions(-)
diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c index 815acf6963916bbfa5069c700360fa2fb2ca4e1e..43df0e1b58793e2efe55ea1317fc1f7ca8cc579f 100644 --- a/src/tests/cmocka/test_dyndns.c +++ b/src/tests/cmocka/test_dyndns.c @@ -355,6 +355,266 @@ void dyndns_test_addr_list_as_str_list(void **state) assert_true(check_leaks_pop(dyndns_test_ctx) == true); }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
Can somebody explain how is this chage rellated to the grouping A addresses into one transaction?
It's not related. It's just a little code style change being in the same function as other changes in this patch.
I'm aware that you recently became passionate about such irrelevant changes, however other developers seems not to mind. Might be you can try to formulate such a rule for our code style guidelines and if agreed to we can adhere to it?
Patch should contain changes which are described in commit message. If there are strong needs for fixing coding style issues in unrelated code then it shoudl be in separate commit message. So someone can immediately identify with git blame that changes are could not affeft functionality or create regression and problem was created before.
I would recommed to read mails more carefully: Could you exaplain what is not clear in following two lines?
Unrelated changes should not be part of another ticket. It just obfluscate output of git blame.
sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); ret = sss_get_dualstack_addresses(dyndns_test_ctx,
@@ -436,7 +696,7 @@ void dyndns_test_dualstack_multiple_addresses(void **state) }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
^
Moreover this change was half done. There is still extra space.
Yes, you are right. I'm just not sure it's worth to send another patch to fix such little problem.
Definitelly no.
LS
On 10/06/2015 10:28 AM, Lukas Slebodnik wrote:
On (06/10/15 10:03), Pavel Reichl wrote:
On 10/06/2015 07:45 AM, Lukas Slebodnik wrote:
On (05/10/15 14:14), Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote:
On 10/05/2015 11:55 AM, Jakub Hrozek wrote: > On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
> Additionally, what versions of nsupdate do /not/ have the realm command? > If they are very old, can we just drop the support for them? (I haven't > checked, just asking..)
It seems to me that support for REALM keyword was introduced in bind 9.9.0 in 'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
From bdd7d705a218d3e2b450ddcefe65b57cd414765d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Resolves: https://fedorahosted.org/sssd/ticket/2495
src/providers/dp_dyndns.c | 56 ++++++--- src/tests/cmocka/test_dyndns.c | 280 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 20 deletions(-)
diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c index 815acf6963916bbfa5069c700360fa2fb2ca4e1e..43df0e1b58793e2efe55ea1317fc1f7ca8cc579f 100644 --- a/src/tests/cmocka/test_dyndns.c +++ b/src/tests/cmocka/test_dyndns.c @@ -355,6 +355,266 @@ void dyndns_test_addr_list_as_str_list(void **state) assert_true(check_leaks_pop(dyndns_test_ctx) == true); }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
Can somebody explain how is this chage rellated to the grouping A addresses into one transaction?
It's not related. It's just a little code style change being in the same function as other changes in this patch.
I'm aware that you recently became passionate about such irrelevant changes, however other developers seems not to mind. Might be you can try to formulate such a rule for our code style guidelines and if agreed to we can adhere to it?
Patch should contain changes which are described in commit message. If there are strong needs for fixing coding style issues in unrelated code
In the discussed case there was not strong need to fix the code style issue. So we should just ignore minor code style issues forever?
then it shoudl be in separate commit message. So someone can immediately identify with git blame that changes are could not affeft functionality or create regression and problem was created before.
I would recommed to read mails more carefully: Could you exaplain what is not clear in following two lines?
Thank you very much for recommendation I'll do my best to stick to it. However I asked you for formulating such a rule for our code style guidelines. By that I thought you might write a new paragraph about rules for patches in http://www.freeipa.org/page/Coding_Style
Unrelated changes should not be part of another ticket. It just obfluscate output of git blame.
sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); ret = sss_get_dualstack_addresses(dyndns_test_ctx,
@@ -436,7 +696,7 @@ void dyndns_test_dualstack_multiple_addresses(void **state) }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
^
Moreover this change was half done. There is still extra space.
Yes, you are right. I'm just not sure it's worth to send another patch to fix such little problem.
Definitelly no.
So why exactly did you mention that?
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (06/10/15 10:40), Pavel Reichl wrote:
On 10/06/2015 10:28 AM, Lukas Slebodnik wrote:
On (06/10/15 10:03), Pavel Reichl wrote:
On 10/06/2015 07:45 AM, Lukas Slebodnik wrote:
On (05/10/15 14:14), Pavel Reichl wrote:
On 10/05/2015 02:08 PM, Petr Spacek wrote:
On 5.10.2015 13:35, Pavel Reichl wrote: > > >On 10/05/2015 11:55 AM, Jakub Hrozek wrote: >>On Thu, Oct 01, 2015 at 01:15:10PM +0200, Pavel Reichl wrote:
[snip]
>>Additionally, what versions of nsupdate do /not/ have the realm command? >>If they are very old, can we just drop the support for them? (I haven't >>checked, just asking..) > >It seems to me that support for REALM keyword was introduced in bind 9.9.0 in >'bug' 2929 (https://kb.isc.org/article/AA-00496/0/). In my opinion this is old >version and we can expect newer version to be present on rhel and fedora.
In fact the option 'realm' was added in BIND 9.8.0a1, which is available in RHEL 6.x. IMHO it is okay to ignore older versions because they would not work (for various reasons) anyway :-)
Petr^2 Spacek
Thanks Petr, I filled #2817 and assigned it to myself.
Updated patches attached.
Thanks!
From bdd7d705a218d3e2b450ddcefe65b57cd414765d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: [PATCH 2/2] DYNDNS: improve nsupdate_msg_add_fwd()
Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to.
It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses.
Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail.
For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
Resolves: https://fedorahosted.org/sssd/ticket/2495
src/providers/dp_dyndns.c | 56 ++++++--- src/tests/cmocka/test_dyndns.c | 280 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 20 deletions(-)
diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c index 815acf6963916bbfa5069c700360fa2fb2ca4e1e..43df0e1b58793e2efe55ea1317fc1f7ca8cc579f 100644 --- a/src/tests/cmocka/test_dyndns.c +++ b/src/tests/cmocka/test_dyndns.c @@ -355,6 +355,266 @@ void dyndns_test_addr_list_as_str_list(void **state) assert_true(check_leaks_pop(dyndns_test_ctx) == true); }
struct sockaddr_in sin;
- memset (&sin, 0, sizeof (sin));
- memset(&sin, 0, sizeof (sin));
Can somebody explain how is this chage rellated to the grouping A addresses into one transaction?
It's not related. It's just a little code style change being in the same function as other changes in this patch.
I'm aware that you recently became passionate about such irrelevant changes, however other developers seems not to mind. Might be you can try to formulate such a rule for our code style guidelines and if agreed to we can adhere to it?
Patch should contain changes which are described in commit message. If there are strong needs for fixing coding style issues in unrelated code
In the discussed case there was not strong need to fix the code style issue. So we should just ignore minor code style issues forever?
then it shoudl be in separate commit message. So someone can immediately identify with git blame that changes are could not affeft functionality or create regression and problem was created before.
I would recommed to read mails more carefully: Could you exaplain what is not clear in following two lines?
Thank you very much for recommendation I'll do my best to stick to it. However I asked you for formulating such a rule for our code style guidelines. By that I thought you might write a new paragraph about rules for patches in http://www.freeipa.org/page/Coding_Style
I hope you understood the message. Feel free to formulate it better. IMHO it needn't be in coding style. BTW freeipa developers do the same with pep8 issues. They fix it if it is part of related work but they do not touch other unrelated warnings in the file or function. The reason is the same: "git blame should not be obfuscated with unrelated changes"
LS
sssd-devel@lists.fedorahosted.org