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

Pavel Reichl preichl at redhat.com
Mon Oct 5 11:35:04 UTC 2015



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.

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) {
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>


More information about the sssd-devel mailing list