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

Jakub Hrozek jhrozek at redhat.com
Tue Aug 11 13:16:59 UTC 2015


On Tue, Aug 11, 2015 at 01:26:47PM +0200, Pavel Reichl wrote:
> 
> On 07/28/2015 01:06 PM, Jakub Hrozek wrote:
> >On Tue, Jul 28, 2015 at 01:02:39PM +0200, Pavel Reichl wrote:
> >>
> >>On 07/24/2015 09:46 PM, Jakub Hrozek wrote:
> >>>>On 24 Jul 2015, at 19:44, Pavel Reichl <preichl at redhat.com> wrote:
> >>>>
> >>>>
> >>>>On 07/24/2015 07:41 PM, Pavel Reichl wrote:
> >>>>>Hello,
> >>>>>
> >>>>>please see attached patch set.
> >>>>>
> >>>>>Thanks!
> >>>>>
> >>>>>
> >>>>>_______________________________________________
> >>>>>sssd-devel mailing list
> >>>>>
> >>>>>sssd-devel at lists.fedorahosted.org
> >>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>Hmm...I should have sent this to the thread "DYNDNS: Add a new option dyndns_server", sorry.
> >>>I think it's fine, rather than re-sending the patchset, please just send a note to that thread.
> >>>
> >>>As a first thing to fix, please come up with a better title for patch #2 :-)
> >>OK, no problem I can do that. I consider this 2nd patch to be ephemera that
> >>will be definitely squashed into 1st patch and thus won't be part of git
> >>history, so the caption doesn't matter at all. I could squash it myself but
> >>I thought it will be easier for reviewer (probably you) to  see your patch
> >>unchanged and my proposed changes as two separate patches. Please correct me
> >>if I'm wrong.
> >>
> >>Anyway I think I can postpone the change until more comments come, right?
> >Sure.
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> Hello, I have rebased patch set, changed name of second patch and commit
> message of 0004-DYNDNS-remove-zone-command.patch/
> 
> Sorry for delay.
> 
> 

> From 4ac6071a397b315957f0257bd0c9467ca48fcec0 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhrozek at redhat.com>
> 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?

> From 618230028bac79daed2447d5ea428bf62ee20c26 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> 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?

> From a92cc49ec5f3d2848ec4a013bd882bafc82c7cb3 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Thu, 23 Jul 2015 04:50:38 -0400
> Subject: [PATCH 3/9] DYNDNS: remove redundant talloc_steal()
> 
> String 'update_msg' was already allocated on mem_ctx, so, there is no
> need to steal it.

ACK

> From 822fb54e81a71138271fa4f273aa9d6c8fea9f01 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Thu, 23 Jul 2015 05:30:34 -0400
> Subject: [PATCH 4/9] DYNDNS: remove zone command
> 
> Remove zone command from message to nsupsate. This command is generally
> used to hint nsupdate. In correctly configured environment such
> information should be obtained via DNS.
> 
> If DNS does not provide necessary information we give other hints.
> 
> For more details see:
> https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2495

ACK

> From ee1c2db6ed6047d56158cad9819dca8074e54f71 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Thu, 23 Jul 2015 09:52:47 -0400
> Subject: [PATCH 5/9] DYNDNS: rename field of sdap_dyndns_update_state
> 
> Rename 'use_server_with_nsupdate' to more general name 'fallback_mode'.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2495

ACK

> From 4807c5d5f641f561d02f3671da36227da181358c 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 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..

> From cc90ec5bedfcbe094c44457c9339035096f50509 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Fri, 24 Jul 2015 08:24:48 -0400
> Subject: [PATCH 7/9] DYNDNS: remove code duplication
> 
> Move copy pasted code for converting sockaddr_storage to string into
> function.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2495

ACK. I was thinking about adding a mem_ctx to the function but we use
the string as an argument for asprintf so the stack-allocated buffer is
probably better.

> From d0a0e516723161504900f05d0504288c73c29000 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Fri, 24 Jul 2015 10:41:42 -0400
> Subject: [PATCH 8/9] TESTS: UT for sss_iface_addr_list_as_str_list()

ACK

> From 02e090379b5ea50578b7f6451e89df0dd418762e 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 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.


> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2495

[...]

> --- a/src/tests/cmocka/test_dyndns.c
> +++ b/src/tests/cmocka/test_dyndns.c
> @@ -328,6 +328,266 @@ void dyndns_test_addr_list_as_str_list(void **state)
>      assert_true(check_leaks_pop(dyndns_test_ctx) == true);
>  }
>  
> +void dyndns_test_create_fwd_msg(void **state)
> +{
> +    errno_t ret;
> +    char *msg;
> +    struct sss_iface_addr *addrlist;
> +    int i;
> +
> +    check_leaks_push(dyndns_test_ctx);
> +
> +    /* getifaddrs is called twice in sss_get_dualstack_addresses() */
> +    for (i = 0; i < 2; i++) {
> +        will_return_getifaddrs("eth0", "192.168.0.2", AF_INET);
> +        will_return_getifaddrs("eth1", "192.168.0.1", AF_INET);
> +        will_return_getifaddrs("eth0", "2001:cdba::555", AF_INET6);
> +        will_return_getifaddrs("eth1", "2001:cdba::444", AF_INET6);
> +        will_return_getifaddrs(NULL, NULL, 0); /* sentinel */
> +    }
> +
> +    struct sockaddr_in sin;
> +    memset (&sin, 0, sizeof (sin));

I don't like the spaces after calling a function, style-wise.
> +    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);




More information about the sssd-devel mailing list