[SSSD] [PATCHES] DYDNDS: update quality of input for nsupdate

Lukas Slebodnik lslebodn at redhat.com
Tue Oct 6 09:42:46 UTC 2015


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 at 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


More information about the sssd-devel mailing list