On 07/07/2011 09:01 PM, Stephen Gallagher wrote:
> On Mon, 2011-07-04 at 19:24 -0400, Jakub Hrozek wrote:
>>
https://fedorahosted.org/sssd/ticket/802
>>
>> [PATCH 1/4] Do not hardcode default resolver timeout
>> This will allow us to reuse the timeout during the dyndns check
>
> Ack.
>
>>
>> [PATCH 2/4] Split reading resolver family order into a separate function
>> The new function will be reused as well
>>
>
> Ack.
>
>> [PATCH 3/4] Allow returning arbitrary address from resolv_hostent as string
>> While we always connect to the first address in the list returned from
>> DNS, the dyndns check code needs the complete list
>>
>
> Ack
>
>> [PATCH 4/4] Check DNS records before updating
>> The check itself.
>>
>
> Nack.
> nitpick: ipa_dyndns_gss_tsig_update_step() has a line:
> DEBUG (7, ("Checking if the update is needed\n"));
> The space between "DEBUG" and "(7," is wrong. (I also note that
there's
> a similar incorrect space in my original code in
> ipa_dyndns_update_send() )
>
> in ipa_dyndns_update_get_addrs_done() if the first address family
> returns ENOENT, you're not checking the second address family.
>
> Also, I'd prefer for readability that you should do:
> if (ret == ENOENT) {
> tevent_req_done(req);
> return;
> } else if (ret != EOK) {
> tevent_req_error(req, ret);
> return;
> }
>
> /* EOK */
> ...
>
> It reads a lot nicer, leaves most of the function one indent level
> further left and doesn't require the dangling tevent_req_error() at the
> end.
>
>> The patch also includes one change that I was wondering about splitting
>> out as a separate patch. The previous code would always delete both
>> address families even in case the updated address was detected from the
>> LDAP socket, so the other address family would disappear if set. This
>> patch changes the behaviour such that only the address family of the
>> ldap socket is deleted and readded (and also checked for in DNS).
>
> This was actually intentional behavior. If the interface isn't
> specified, we ONLY want to have the address that's connected to the LDAP
> server stored. It's intentional that the other address family was
> removed in this situation (since it may be a leftover from a connection
> while on a different network that supported the other family).
>
>
Thans for the review, new patches are attached.
Only patch 4 has changed, though.