[SSSD] [PATCH] Check DNS records before updating

Jakub Hrozek jhrozek at redhat.com
Mon Jul 11 12:57:32 UTC 2011


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Do-not-hardcode-default-resolver-timeout.patch
Type: text/x-patch
Size: 1324 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110711/f81d6b8c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Split-reading-resolver-family-order-into-a-separate-.patch
Type: text/x-patch
Size: 4508 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110711/f81d6b8c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Allow-returning-arbitrary-address-from-resolv_hosten.patch
Type: text/x-patch
Size: 2176 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110711/f81d6b8c/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Check-DNS-records-before-updating.patch
Type: text/x-patch
Size: 20574 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110711/f81d6b8c/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110711/f81d6b8c/attachment.sig>


More information about the sssd-devel mailing list