[SSSD] [PATCH] Make the dynamic DNS updates backend-agnostic

Jakub Hrozek jhrozek at redhat.com
Fri May 3 18:25:22 UTC 2013


On Fri, May 03, 2013 at 08:17:04PM +0200, Lukas Slebodnik wrote:
> On (03/05/13 16:31), Jakub Hrozek wrote:
> >On Thu, May 02, 2013 at 12:49:06PM +0200, Jakub Hrozek wrote:
> >> On Thu, May 02, 2013 at 10:33:06AM +0200, Pavel Březina wrote:
> >> > >>Nack.
> >> > >>
> >> > >>Mostly minor things.
> >> > >>
> >> > >>ok_for_dns(): - use switch instead of if-elseif-else
> >> > >>
> >> > >
> >> > >Done
> >> > >
> >> > >>sss_iface_addr_list_get(): - missing space around = in for -
> >> > >>missing space after if (last two)
> >> > >>
> >> > >
> >> > >Done
> >> > 
> >> > +            address->addr = talloc_memdup(address, ifa->ifa_addr,
> >> > +                                          addrsize);
> >> > +            if(address->addr == NULL) {
> >> >                ^ you missed one
> >> > 
> >> > +                ret = ENOMEM;
> >> > +                goto done;
> >> > +            }
> >> > 
> >> 
> >> Fixed too.
> >> 
> >> > >>be_nsupdate_create_msg(): - please add a comment that
> >> > >>realm_directive ends with \n so using %szone format is correct -
> >> > >>you should use tmp_ctx - inet_ntop sets errno, so you should use
> >> > >>its value instead of EIO
> >> > >>
> >> > >
> >> > >Done
> >> > >
> >> > >>struct nsupdate_get_addrs_state: - family_order is already present
> >> > >>in be_res and it is set on line 420 state->family_order =
> >> > >>be_res->family_order;
> >> > >>
> >> > >
> >> > >Removed, the code now uses the order from be_res.
> >> > >
> >> > >>nsupdate_get_addrs_send(): - please, strdup hostname hostname is an
> >> > >>error prone variable because it is often placed on stack, when it
> >> > >>comes from gethostname()
> >> > >
> >> > >Done
> >> > >
> >> > >>
> >> > >>nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate
> >> > >>that the request continues and ret != EAGAIN to finish the request,
> >> > >>instead of calling tevent_req_done() and tevent_req_error() on
> >> > >>several places
> >> > >>
> >> > >
> >> > >Almost done. I kept using return after tevent_req_set_callback. It
> >> > >seems clearer to me than "ret = EAGAIN; goto done;".
> >> > 
> >> > OK. I'm fine with that.
> >> > 
> >> > >>- line 472: fall back between IP versions is already implemented
> >> > >>in resolver. So if it returns ENOENT here, you already know that
> >> > >>you cannot resolve the hostname, no need to retry with other IP
> >> > >>version.
> >> > >>
> >> > >
> >> > >Done. I guess the fallback in resolver predates the original dyndns
> >> > >code.
> >> > >
> >> > >>- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
> >> > >>not tell you what version is in rhostent. It is because the fall
> >> > >>back to next family is already implemented in resolver.
> >> > >
> >> > >As discussed off-list this is really needed as the resolver only
> >> > >falls back in case the first address family does not find anything.
> >> > 
> >> > Besides the space, it's code-wise ack. I will test all dyndns patches at
> >> > ones.
> >> > 
> >> > Just one thing to keep in mind for the future: please, do not use both
> >> > !ptr and ptr == NULL conditions inside one function. Choose one and
> >> > stick with it. I prefer the latter, because pointer is not a boolean value.
> >> 
> >> I skimmed through the code and fixed a couple of cases I found. I prefer
> >> ptr == NULL, too.
> >> 
> >> New patches attached.
> >
> >Lukas spotted a possibly uninitialized variable in the unit tests. The
> >attached patches fix that.
> 
> Patches have been tested with AD dyn dns updates.
> 
> Ack

Thank you for the review.

Pushed to master.



More information about the sssd-devel mailing list