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

[Pavel Reichl]

[snip]
> > From 4ac6071a397b315957f0257bd0c9467ca48fcec0 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sun, 6 Jul 2014 22:53:27 +0200
> > Subject: [PATCH 1/9] DYNDNS: Add a new option dyndns_server
>
> Can you review this patch?
> +++ b/src/man/sssd-ad.5.xml
> @@ -812,6 +812,26 @@ ad_gpo_map_deny = +my_pam_service
>                      </listitem>
>                  </varlistentry>
>            <varlistentry>
>                <term>dyndns_server (string)</term>
>                <listitem>
>                    <para>
>                        The DNS server to use when performing a DNS
>                        update. In most setups, it's recommended 
>
> to leave
>                        this option unset.
>                    </para>
>                    <para>
>                        Setting this option makes sense for 
>
> environments
>                        where the DNS server is different from 
>
> the identity
>                        server.
>                    </para>
>                    <para>
>                        Default: None (let nsupdate choose the 
>
> server)
>                    </para>
>                </listitem>
>            </varlistentry>

One empty line too many?
>              <xi:include 
>
> xmlns:xi="http://www.w3.org/2001/XInclude" 
> href="include/override_homedir.xml" />
>                  <xi:include 
> xmlns:xi="http://www.w3.org/2001/XInclude" 
> href="include/homedir_substring.xml" />

ACK, I'm not running CI, I would prefer to run in on whole patch set.

I was thinking whether we should amend the man page to state explicitly 
that dyndns_server is not used in the first attempt to perform DNS 
udpate, but only in the subsequent (failover) attempt that contains such 
hints. However, I suppose that such change should be part of the patches 
that actually change this behavior...or we could be silent about this...?
> > From 618230028bac79daed2447d5ea428bf62ee20c26 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Thu, 23 Jul 2015 04:40:03 -0400
> > Subject: [PATCH 2/9] DYNDNS: Don't use server cmd in nsupdate by default
> >
> > https://fedorahosted.org/sssd/ticket/2495
>
> ACK, but can you add some explanation to the commit message?

done
[snip]
> > From 4807c5d5f641f561d02f3671da36227da181358c Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Thu, 23 Jul 2015 10:51:50 -0400
> > Subject: [PATCH 6/9] DYNDNS: use realm and server commands only as fallback
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2195
>
> [...]
> > diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> > index ae3f913ee392a6513f75aab497e7f2d784784748..efa6d73dcf66cb685cf3f331b38e75b5435aead1 100644
> > --- a/src/providers/dp_dyndns.c
> > +++ b/src/providers/dp_dyndns.c
> > @@ -394,11 +394,16 @@ nsupdate_msg_create_common(TALLOC_CTX *mem_ctx, const char *realm,
> >       tmp_ctx = talloc_new(NULL);
> >       if (tmp_ctx == NULL) return NULL;
> > if (realm != NULL) {
> > #ifdef HAVE_NSUPDATE_REALM
> >  realm_directive = talloc_asprintf(tmp_ctx, "realm %s\n", realm);
> > #else
> >  realm_directive = talloc_asprintf(tmp_ctx, "\n");
> > #endif
> > } else {
> >    realm_directive = talloc_asprintf(tmp_ctx, "\n");
> > }
>
> Please move this block into a function..

done
[snip]
> > From 02e090379b5ea50578b7f6451e89df0dd418762e Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Fri, 24 Jul 2015 13:25:56 -0400
> > Subject: [PATCH 9/9] DYNDNS: fix nsupdate_msg_add_fwd()
> >
> > Update nsupdate_msg_add_fwd() to group commands by address family
> > processed IP address belongs to.
>
> Please reword the commit message, the "fix" part sounds scary. Please
> also add the explanation why the separate sends are preferable.

Done.
[snip]
> struct sockaddr_in sin;
> memset (&sin, 0, sizeof (sin));
>
> I don't like the spaces after calling a function, style-wise.

done
> > sin.sin_family = AF_INET;
> > sin.sin_addr.s_addr = inet_addr ("192.168.0.2");
> > ret = sss_get_dualstack_addresses(dyndns_test_ctx,
> >                                  (struct sockaddr *) &sin,
> >                                  &addrlist);
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel