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

Lukas Slebodnik lslebodn at redhat.com
Mon Oct 5 12:12:15 UTC 2015


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


More information about the sssd-devel mailing list