I pushed a multitude of trivial fixes and tweaks for comments, code formatting and documentation as it is quicker to fix them then to describe them. The rest of my remarks follows. All line numbers below are taken from source files as present in the current head of the rt45186 branch, i.e. at commit 632ea77d59. Code issues ----------- o Commit fae626c51e1 incorrectly refactors NS_SERVER_DISABLE{4,6} checks in bin/named/server.c by reversing their meaning. o Commit ca4a44cc557 causes a variable in host byte order to be assigned to network byte order fields (sin{,6}_port) without calling htons(). o Commit ca4a44cc55 migrates nsupdate to libirs in a way which will change nsupdate's behavior after rt45186 is rebased on top of current master. Specifically, current master prevents IPv4 name servers in /etc/resolv.conf from being used when -6 is passed to nsupdate (and vice versa for IPv6 name servers with -4). Applying ca4a44cc55 on top of current master allows any name server found in /etc/resolv.conf to be used, no matter whether -4/-6 is passed to nsupdate. o Commit 327794c468 introduces ns_server_setserverid(), but does not check its return value in one branch inside load_configuration(). The simplest solution is to perform a single RUNTIME_CHECK() below the relevant sequence of conditional statements instead of repeating that check inside each branch. Possible improvements --------------------- o libirs migration in commit 9f4e5e277c makes the code block on lines 1375-1386 of bin/dig/dighost.c redundant as libirs automatically falls back to localhost when /etc/resolv.conf does not contain any name servers and none are supplied explicitly (see lines 557-564 of lib/irs/resconf.c). o Regarding the previous note, libirs could arguably prefer ::1 over 127.0.0.1 when falling back to localhost. o Function ns__client_request() in lib/ns/client.c contains quite some "goto cleanup" statements, but the only thing after the "cleanup" label is a return statement. Replacing these goto statements with return statements and removing the "cleanup" label (along with the return statement following it) would improve readability. o In function ns__client_request() in lib/ns/client.c, how about removing the local variable "view" and calling matchingview() on line 2583 with &client->view as the last argument? o Commit 6255f2e332 causes two functions - isc_mem_printallactive() and isc_mem_checkdestroyed() - to contain the exact same loop with a non-trivial body. Extracting that loop into a separate static function would improve readability. o Commit 3cc97da7fe adds a "client" local variable to function query_sortlist_order_1element(). This variable can be used in that function instead of all occurences of "qctx->client". o It is probably a good idea to replace "ns_notify_start" in unit test lib/ns/tests/notify_test.c with something else (or to add a suffix to it). It took me a while to figure out that the call to ns_notify_start() on line 144 executes a function inside lib/ns/client.c and not something inside the unit test itself. Doubts and questions -------------------- o Do we really need to #define NS_CLIENTATTR_CALLBACK? Checking client->sendcb would be enough for client_send() to figure out that it should run a callback instead of actually sending a response. o Consider the following code in ns__clientmgr_getclient(): 3611 client = NULL; 3612 ISC_QUEUE_POP(manager->inactive, ilink, client); 3613 if (client != NULL) 3614 MTRACE("getclient (recycle)"); 3615 else { 3616 MTRACE("getclient (create)"); 3617 3618 LOCK(&manager->lock); 3619 result = client_create(manager, &client); ... 3627 } 3628 3629 client->manager = manager; 3630 ns_interface_attach(ifp, &client->interface); 3631 client->state = NS_CLIENTSTATE_READY; 3632 client->sctx = manager->sctx; 3633 INSIST(client->recursionquota == NULL); Is the assignment on line 3632 really necessary? If a new client is created, client_create() calls ns_server_attach(), which performs the same assignment. Recycled clients should also have sctx set. Am I missing something? Similar assignments also happen in get_client() (line 3505) and get_worker() (line 3570). o What benefit do the non-NULL checks in commit 3eb5aad6e9 bring? I was unable to figure out a code path which would cause these checks to evaluate to false. Also, there are other instances of ns_server_detach() calls without a similar non-NULL check (e.g. in clientmgr_destroy() in lib/ns/client.c). o Commit addd8a38b2 extends ns_interfacemgr_getaclenv() with a REQUIRE() assertion. However, some other ns_interfacemgr_*() functions in lib/ns/interfacemgr.c still lack that check. Is there any reason ns_interfacemgr_getaclenv() was singled out? o Commit c336ed47ba moves the isself() callback back into bin/named/zoneconf.c. However, I was unable to quickly figure out why that is needed or desired. o Commit cfb4fc12f8 "incidentally fixes a potential problem with dns_aclenv_destroy()". Could you please explain what that potential problem is? If you discovered a code path which causes erroneuous behavior, can it be reproduced in a test? o Commit 98a019e06a adds function prototypes to lib/ns/tests/nstest.h, commenting them as "some test functions defined in lib/isc but not in include files". However, the same commit adds the same function prototypes to lib/isc/include/isc/task.h. BIND still builds and tests work properly even after removing these prototypes from lib/ns/tests/nstest.h. o Commit 76be714256 adds the following typedef: typedef void (*isc_fuzz_callback)(void); This typedef is not used anywhere either before or in rt45186. Miscellaneous ------------- o A lot of function prototypes in lib/ns/include/ns/ are not commented: * lib/ns/include/ns/client.h: - ns_client_log() - ns_client_logv() - ns_client_aclmsg() - ns_client_sourceip() - ns_client_addopt() - ns__clientmgr_getclient() - ns__client_request() * lib/ns/include/ns/interfacemgr.h: - ns_interfacemgr_attach() - ns_interfacemgr_detach() - ns_interfacemgr_shutdown() - ns_interfacemgr_getaclenv() - ns_interface_attach() - ns_interface_detach() - ns_interfacemgr_dumprecursing() - ns_interfacemgr_listeningon() - ns__interfacemgr_getif() - ns__interfacemgr_nextif() * lib/ns/include/ns/query.h: - ns_query_init() - ns_query_free() - ns_query_start() - ns_query_cancel() * lib/ns/include/ns/server.h - ns_server_settimeouts() * lib/ns/include/ns/stats.h: - ns_stats_attach() - ns_stats_detach() - ns_stats_create() - ns_stats_increment() - ns_stats_decrement() - ns_stats_get() * lib/ns/include/ns/update.h: - ns_update_start() * lib/ns/include/ns/xfrout.h: - ns_xfr_start() o Consider the following code in lib/ns/client.c: 3597 isc_result_t 3598 ns__clientmgr_getclient(ns_clientmgr_t *manager, ns_interface_t *ifp, 3599 isc_boolean_t tcp, ns_client_t **clientp) 3600 { 3601 isc_result_t result = ISC_R_SUCCESS; 3602 ns_client_t *client; 3603 MTRACE("getclient"); 3604 3605 REQUIRE(clientp != NULL && *clientp == NULL); 3606 REQUIRE(manager != NULL); Not sure about this one, but do we not want to use VALID_MANAGER() on line 3606 instead of a simple non-NULL check? o In unit test lib/ns/tests/listenlist_test.c: * ATF_REQUIRE_EQ() should probably be used instead of ATF_CHECK_EQ() for checking the result of ns_listenlist_default(). I do not see any point in checking anything further if that call fails. * You might want to ATF_REQUIRE() "list" to be non-NULL before dereferencing it. * Consider moving the assigment of 0 to "count" from line 39 to somewhere just before the first while loop, for consistency with the second while loop. o I find the naming of some functions in lib/ns/server.c inconsistent: ns_server_option() ns_server_setoption() vs. ns_server_gettimeouts() ns_server_settimeouts() I like the latter scheme more, but use your own judgement. o LIBISC_EXTERNAL_DATA is used instead of LIBNS_EXTERNAL_DATA in lib/ns/include/ns/version.h. o LIBDNS_EXTERNAL_DATA is used instead of LIBNS_EXTERNAL_DATA for ns_lctx in lib/ns/log.c. o Commits 06839d3a66 and 2e63de5ae5 use "void const" while all other code in the tree uses "const void". o Commit 9a2996fe44 claims to have removed all references to lwres, but leaves one in doc/misc/options. o There is no rt45186_base tag.