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