On (03/05/13 16:31), Jakub Hrozek wrote:
On Thu, May 02, 2013 at 12:49:06PM +0200, Jakub Hrozek wrote:
> On Thu, May 02, 2013 at 10:33:06AM +0200, Pavel Březina wrote:
> > >>Nack.
> > >>
> > >>Mostly minor things.
> > >>
> > >>ok_for_dns(): - use switch instead of if-elseif-else
> > >>
> > >
> > >Done
> > >
> > >>sss_iface_addr_list_get(): - missing space around = in for -
> > >>missing space after if (last two)
> > >>
> > >
> > >Done
> >
> > + address->addr = talloc_memdup(address, ifa->ifa_addr,
> > + addrsize);
> > + if(address->addr == NULL) {
> > ^ you missed one
> >
> > + ret = ENOMEM;
> > + goto done;
> > + }
> >
>
> Fixed too.
>
> > >>be_nsupdate_create_msg(): - please add a comment that
> > >>realm_directive ends with \n so using %szone format is correct -
> > >>you should use tmp_ctx - inet_ntop sets errno, so you should use
> > >>its value instead of EIO
> > >>
> > >
> > >Done
> > >
> > >>struct nsupdate_get_addrs_state: - family_order is already present
> > >>in be_res and it is set on line 420 state->family_order =
> > >>be_res->family_order;
> > >>
> > >
> > >Removed, the code now uses the order from be_res.
> > >
> > >>nsupdate_get_addrs_send(): - please, strdup hostname hostname is an
> > >>error prone variable because it is often placed on stack, when it
> > >>comes from gethostname()
> > >
> > >Done
> > >
> > >>
> > >>nsupdate_get_addrs_done(): - please use ret = EAGAIN to indicate
> > >>that the request continues and ret != EAGAIN to finish the request,
> > >>instead of calling tevent_req_done() and tevent_req_error() on
> > >>several places
> > >>
> > >
> > >Almost done. I kept using return after tevent_req_set_callback. It
> > >seems clearer to me than "ret = EAGAIN; goto done;".
> >
> > OK. I'm fine with that.
> >
> > >>- line 472: fall back between IP versions is already implemented
> > >>in resolver. So if it returns ENOENT here, you already know that
> > >>you cannot resolve the hostname, no need to retry with other IP
> > >>version.
> > >>
> > >
> > >Done. I guess the fallback in resolver predates the original dyndns
> > >code.
> > >
> > >>- line 539: if resolver returned EOK, IPV4_FIRST || IPV6_FIRST does
> > >>not tell you what version is in rhostent. It is because the fall
> > >>back to next family is already implemented in resolver.
> > >
> > >As discussed off-list this is really needed as the resolver only
> > >falls back in case the first address family does not find anything.
> >
> > Besides the space, it's code-wise ack. I will test all dyndns patches at
> > ones.
> >
> > Just one thing to keep in mind for the future: please, do not use both
> > !ptr and ptr == NULL conditions inside one function. Choose one and
> > stick with it. I prefer the latter, because pointer is not a boolean value.
>
> I skimmed through the code and fixed a couple of cases I found. I prefer
> ptr == NULL, too.
>
> New patches attached.
Lukas spotted a possibly uninitialized variable in the unit tests. The
attached patches fix that.
Patches have been tested with AD dyn dns updates.
Ack