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

Jan Zelený jzeleny at redhat.com
Wed Jun 15 14:08:20 UTC 2011


> On 06/12/2011 11:27 AM, Jakub Hrozek wrote:
> > 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.
> 
> Sorry for the churn, but new patches are attached. Only patches 0002 and
> 0004 are changed.
> 
> 0002 has been simplified and 0004 now exposes the hosts databases. I
> will be using it in the upcoming dyndns patches anyway, so I'm squashing
> the change here to avoid another unnecessary patch.

Just one minor comment:
in function resolv_gethostbyname_dns_parse() you use redundant assignments to 
variables addrttls and addr6ttls. Other than that all patches seem and work 
fine.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110615/9041a229/attachment.sig>


More information about the sssd-devel mailing list