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.