Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> 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
Ack to all four
Jan