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

Jakub Hrozek jhrozek at redhat.com
Mon Oct 5 09:55:34 UTC 2015


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:

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

> +    /* Remove existing entries as needed */
>      if (remove_af & DYNDNS_REMOVE_AAAA) {


More information about the sssd-devel mailing list