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.