On Fri, Apr 19, 2013 at 01:43:42PM +0200, Pavel Březina wrote:
On 04/17/2013 08:55 PM, Jakub Hrozek wrote:
>On Mon, Apr 15, 2013 at 04:44:53PM +0200, Jakub Hrozek wrote:
>>On Thu, Apr 11, 2013 at 10:16:52AM +0200, Jakub Hrozek wrote:
>>>These two patches depend on patches in that decouple the memory checking
>>>from the check tool.
>>>
>>>[PATCH 1/2] Refactor dynamic DNS updates
>>>Provides two new layers instead of the previous IPA specific layer
>>>
>>>1) dp_dyndns.c -- a very generic dyndns layer on the DP level. Its purpose
>>>it to make it possible for any back end to use dynamic DNS updates.
>>>
>>>2) sdap_dyndns.c -- a wrapper around dp_dyndns.c that utilizes some
>>>LDAP-specific features like autodetecting the address from the LDAP
>>>connection.
>>>
>>>Also converts the dyndns code to new specific error codes.
>>>
>>>The DP layer includes unit tests that also make sure that
>>>https://fedorahosted.org/sssd/ticket/1802 is fixed.
>>>
>>>[PATCH 2/2] Convert IPA-specific options to be back-end agnostic
>>>This patch introduces new options for dynamic DNS updates that are not
>>>specific to any back end. The current ipa dyndns options are still
>>>usable, just with a deprecation warning.
>>>
>>>These options will be reused in the AD dynamic DNS updates code.
>>
>>During testing, I found some places where the patches could be amended.
>>Attached are new versions.
>
>The attached patches fix some typos, functionality is the same as the
>previous iteration.
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
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;".
- 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.