[SSSD] [PATCH] Store and use TTL values for resolved host names

Jakub Hrozek jhrozek at redhat.com
Sun Jun 12 09:27:05 UTC 2011


On 06/09/2011 03:42 PM, Jan Zelený wrote:
>> https://fedorahosted.org/sssd/ticket/811
>>
>> Much of the patch is a conversion from system "struct hostent" to our
>> own "struct resolv_hostent".
>>
>> After some going back and forth, the addresses are stored pretty much
>> same as in "struct hostent" in a uint8_t buffer. The reason being
>> simpler access to the address as seen in the inet_ntop calls without
>> having to explicitly specify v6 or v4 component of a union, for example.
>> The "family" structure member specifies the address family which implies
>> the address type after all.
>>
>> For resolving hostnames from /etc/hosts, I still used
>> ares_gethostbyname() with databases set to "files" only. It is
>> convenient do to so, as they implement the parser and provide a nice
>> interface. The only issue I came across was that if a hostname is not
>> found in /etc/hosts, c-ares would return ARES_ECONNREFUSED. I consider
>> this a c-ares bug and will handle this upstream.
>
> Nack,
> I have some comments and suggestions. Please note that line numbers are taken
> from the patched file.
>
> <nitpicking>
> 440, 483, 531 - return NULL should be enough, no need to jump
> </nitpicking>
> 504 - the ->ipaddr missing?
> 615 - condition doesn't make any sense
> 617 - missing attribute
> 840 - throw this block out? IMO it's redundant
> 857 - I'd much rather see != instead of<
>
> 926 - maybe one line comment about what does the block do or under what
> circumstances can it be invoked?
>
> 933 - I'd move this above the previous block and in the block add the timeout
> again (similar situation is around line 847s in case the block on line 840s
> will stay). In current state of the code, the timeout would be shared by two
> queries in case the first one is interrupted.
>
> 971 - I'd merge functions resolv_gethostbyname_dns_done and
> resolv_gethostbyname_next_dns_done, they are almost identical
>
> Jan

The review (both on list and explaining the code in person) made me 
realize that keeping the whole gethostbyname logic in one big request 
makes the code really hard to read. Also the family/database logic was 
too hardcoded.

So I reworked the patch, split it into smaller ones which are hopefully 
easier to grasp. The gethostbyname request is now a wrapper that only 
decides which hosts database and/or family to use next and fires 
database-specific subrequests.

The comments from the review were mostly taken into account except where 
I rewrote the code in question.

Patch 0001:
Splits the resolv_hostent structure and the copy routines into a 
separate patch

Patch 0002:
Splits resolving from files into a separate subrequest

Patch 0002:
Splits resolving from DNS into a separate subrequest

PATCH 0004:
Moves the gethostbyname top level request from using ares_gethostbyname 
to using the database-specific subrequests.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-new-resolv_hostent-data-structure-and-utility-fu.patch
Type: text/x-patch
Size: 7277 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110612/e9a1db17/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Resolve-hosts-by-name-from-files-into-resolv_hostent.patch
Type: text/x-patch
Size: 5797 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110612/e9a1db17/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Resolve-hosts-by-name-from-DNS-into-resolv_hostent.patch
Type: text/x-patch
Size: 9186 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110612/e9a1db17/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Switch-resolver-to-using-resolv_hostent-and-honor-TT.patch
Type: text/x-patch
Size: 40813 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110612/e9a1db17/attachment-0003.bin>


More information about the sssd-devel mailing list