On 04/30/2013 04:07 PM, Jakub Hrozek wrote:
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
+ address->addr = talloc_memdup(address, ifa->ifa_addr,
+ addrsize);
+ if(address->addr == NULL) {
^ you missed one
+ ret = ENOMEM;
+ goto 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;".
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.