On Mon, 2011-07-11 at 14:57 +0200, Jakub Hrozek wrote:
> On 07/07/2011 09:01 PM, Stephen Gallagher wrote:
> > On Mon, 2011-07-04 at 19:24 -0400, Jakub Hrozek wrote:
> >>
https://fedorahosted.org/sssd/ticket/802
> >>
> >> [PATCH 1/4] Do not hardcode default resolver timeout
> >> This will allow us to reuse the timeout during the dyndns check
> >
> > Ack.
> >
> >>
> >> [PATCH 2/4] Split reading resolver family order into a separate function
> >> The new function will be reused as well
> >>
> >
> > Ack.
> >
> >> [PATCH 3/4] Allow returning arbitrary address from resolv_hostent as
string
> >> While we always connect to the first address in the list returned from
> >> DNS, the dyndns check code needs the complete list
> >>
> >
> > Ack
> >
> >> [PATCH 4/4] Check DNS records before updating
> >> The check itself.
> >>
> >
> > Nack.
> > nitpick: ipa_dyndns_gss_tsig_update_step() has a line:
> > DEBUG (7, ("Checking if the update is needed\n"));
> > The space between "DEBUG" and "(7," is wrong. (I also note
that there's
> > a similar incorrect space in my original code in
> > ipa_dyndns_update_send() )
> >
> > in ipa_dyndns_update_get_addrs_done() if the first address family
> > returns ENOENT, you're not checking the second address family.
> >
> > Also, I'd prefer for readability that you should do:
> > if (ret == ENOENT) {
> > tevent_req_done(req);
> > return;
> > } else if (ret != EOK) {
> > tevent_req_error(req, ret);
> > return;
> > }
> >
> > /* EOK */
> > ...
> >
> > It reads a lot nicer, leaves most of the function one indent level
> > further left and doesn't require the dangling tevent_req_error() at the
> > end.
> >
> >> The patch also includes one change that I was wondering about splitting
> >> out as a separate patch. The previous code would always delete both
> >> address families even in case the updated address was detected from the
> >> LDAP socket, so the other address family would disappear if set. This
> >> patch changes the behaviour such that only the address family of the
> >> ldap socket is deleted and readded (and also checked for in DNS).
> >
> > This was actually intentional behavior. If the interface isn't
> > specified, we ONLY want to have the address that's connected to the LDAP
> > server stored. It's intentional that the other address family was
> > removed in this situation (since it may be a leftover from a connection
> > while on a different network that supported the other family).
> >
> >
>
> Thans for the review, new patches are attached.
>
> Only patch 4 has changed, though.
Ack.