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

Jakub Hrozek jhrozek at redhat.com
Tue Oct 6 06:33:05 UTC 2015


On Tue, Oct 06, 2015 at 07:39:24AM +0200, Lukas Slebodnik wrote:
> On (05/10/15 20:58), Jakub Hrozek wrote:
> >On Mon, Oct 05, 2015 at 02:12:15PM +0200, Lukas Slebodnik wrote:
> >> 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.
> >
> >Do they really ship so obsolete package versions or are they a
> >minimalistic environment that strips features?
> >
> >Does it make sense to support distributions like this with SSSD master?
> >I mean, there's no way for us to test such codepaths anyway..for the
> >specific realm example, it's not a big deal, but for others it might..
> What are consequences if the realm is not supported and we remove
> ifdef?

We'll try to execute nsupdate with a parameter it doesn't know, which
will blow up, nsupdate won't work.


More information about the sssd-devel mailing list