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

Petr Spacek pspacek at redhat.com
Mon Oct 5 12:08:04 UTC 2015


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 at 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 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
>>
>> [...]
>>
>>> +    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) {


More information about the sssd-devel mailing list