On 06/15/2011 04:08 PM, Jan Zelený wrote:
>> 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
Thank you, new patches attached