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.