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

Lukas Slebodnik lslebodn at redhat.com
Fri May 3 18:17:04 UTC 2013


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



More information about the sssd-devel mailing list