Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: binary X-Mailer: MIME-tools 5.508 (Entity 5.508) Content-Disposition: inline MIME-Version: 1.0 References: X-RT-Original-Encoding: utf-8 Message-ID: In-Reply-To: X-RT-Interface: Web RT-Send-CC: Content-Length: 9523 (All line numbers below are taken from lib/dns/resolver.c as present in the current head of the rt45362 branch, i.e. at commit 8128863942.) As for bugs introduced by refactoring, I only found a minor one related to error handling: rctx_referral() can return more result codes than just ISC_R_SUCCESS and ISC_R_COMPLETE; such result codes are not properly handled by rctx_noanswer(). I also noticed a glitch in BADCOOKIE handling which was already present in the code before refactoring: 8417 } else if (fctx->rmessage->rcode == dns_rcode_badcookie && 8418 fctx->rmessage->cc_ok) { 8419 /* 8420 * We have recorded the new cookie. 8421 */ 8422 if (BADCOOKIE(query->addrinfo)) 8423 query->options |= DNS_FETCHOPT_TCP; 8424 query->addrinfo->flags |= FCTX_ADDRINFO_BADCOOKIE; 8425 rctx->resend = ISC_TRUE; I have not tested this, but it looks like the logical OR on line 8423 should be applied to rctx->queryopts rather than query->options. Otherwise, this operation will be effectively ignored because rctx_done() will call rctx_resend(), which in turn will call fctx_query() with rctx->queryopts as the value of the options argument. Finally, could you please elaborate a bit on the system test changes in f9cd680492? I understand it fixes some problem with the dnssec system test, but it is not immediately obvious to me from looking at the fix what that problem was in the first place, plus a whole check is removed by that commit. All that follows are purely suggestions/comments/bikeshedding. o The choice of rctx fields which are explicitly initialized in rctx_initanswer() seems a bit arbitrary. The memory chunk used by rctx is zeroed by a memset() call inside rctx_respinit(), but rctx_initanswer() still sets some pointer fields to NULL while e.g. not setting negative_response to ISC_FALSE explicitly. One possible strategy here is to only explicitly initialize enums (for clarity as to what the default value is) and skip the rest. o Related to the above, I personally do not feel the explicit "rctx.options = 0;" assignment before the rctx_noanswer() call in resquery_response() is needed. o Surrounding single-line conditional branches with curly braces is inconsistent with both the coding style guidelines on the wiki and the rest of lib/dns/resolver.c code. (Note that I agree with making exceptions e.g. in case of multi-line conditional expressions.) Am I missing some context? o As RCTX_{NS,GLUE}_IN_ANSWER are the only flags defined for the options field of struct respctx, how about converting them into two separate booleans (similar to nextitem and next_server) for consistency? o As dns_message_firstname() and dns_message_nextname() can only return ISC_R_NOMORE or ISC_R_SUCCESS, the conditional expressions that check the results of these calls in rctx_scanauth_neg() and rctx_scanauth_dnssec() could be tweaked a bit to avoid confusion. (I thought the while loops in rctx_scanauth_neg() and rctx_scanauth_dnssec() were incorrectly refactored until I checked what result codes can be returned by dns_message_nextname().) My idea would be to do it like this: isc_boolean_t finished = ISC_FALSE; ... result = dns_message_firstname(fctx->rmessage, section); if (result != ISC_R_SUCCESS) return (ISC_R_SUCCESS); while (!finished) { ... result = dns_message_nextname(fctx->rmessage, section); if (result != ISC_R_SUCCESS) finished = ISC_TRUE; ... } o I see further refactoring opportunities in rctx_parse(), rctx_badserver(), rctx_lameserver() and rctx_delonly(): most of the code inside these functions could be unindented by one level by returning early when the relevant logical condition is not met. o Is the "&& !rctx->chaining" part of the logical condition on line 7225 necessary? This is inside the rctx_scanauth() function, which is called from rctx_answer(). But if rctx->chaining == ISC_TRUE on line 7302, rctx_answer() returns early, preventing rctx_scanauth() from ever being called. Thus, if rctx_scanauth() does get called, it implies that rctx->chaining == ISC_FALSE. I do not see any code which could cause the value of rctx->chaining to change between these two sites. (This issue was also present in the code before refactoring.) o "query->options" vs. "rctx->queryopts" could be used a bit more consistently. It seems both are used interchangeably when checking whether a given fetch option was set for the original query while only the latter can be used for changing fetch options for a retried query. Thus, could we perhaps always use "query->options" for bitwise fetch option checks for the original query? This could probably help with preventing bugs like the one in BADCOOKIE handling mentioned above. (This issue was also present in the code before refactoring.) o Roughly a half of the functions introduced by refactoring (rctx_initanswer(), rctx_scananswer(), rctx_any(), rctx_finalanswer(), rctx_cname(), rctx_dname(), rctx_answer(), rctx_respinit(), rctx_nextserver(), rctx_chaseds(), rctx_done(), rctx_edns(), rctx_parse(), rctx_delegation()) have no associated comments which would describe their purpose. Even a couple of words for each of them would already be something. o Naming trivia: - I find the names of rctx_{answer,finalanswer}() functions a tad confusing. Could we perhaps rename functions like this: rctx_any() -> rctx_answer_any() rctx_finalanswer() -> rctx_answer_final() rctx_cname() -> rctx_answer_cname() rctx_dname() -> rctx_answer_dname() or something among these lines? - Could we perhaps rename rctx_scanauth_dnssec() to rctx_scanauth_neg_dnssec() or something similar? Both rctx_scanauth_neg() and rctx_scanauth_dnssec() are only called from rctx_noanswer() while current naming might suggest that rctx_scanauth_dnssec() can be used for a positive answer as well. - Could we perhaps rename rctx_delonly() to something that includes "delegation" or "deleg"? "delonly" made me think of deleting things, which is not what happens in this function. o Comment trivia: - Field comments inside struct respctx do not match our coding style guidelines. - I would rephrase: Examine the authority section for a positive answer (if there is one). to: Examine the authority section (if there is one) for a positive answer. - s/inavalid response/invalid response/; - Indefinite articles in the comment block starting with "A negative response..." (line 7723) could use some tweaking. (This issue was also present in the code before refactoring.) - Commit 4c36271b11 removed an "XXXRTH log." comment from the code branch handling responses from broken servers, but there is still no logging code in that branch. - In the same code branch, we can see the following code: 8427 rctx->broken_server = DNS_R_UNEXPECTEDRCODE; 8428 INSIST(rctx->broken_server != ISC_R_SUCCESS); 8429 rctx->next_server = ISC_TRUE; I fail to understand what is the point of the INSIST assertion on line 8428. (This issue was also present in the code before refactoring.) o Formatting trivia: - In the sequence of conditional checks inside rctx_done(), either the "result == DNS_R_CHASEDSSERVERS" branch could be moved higher up or the previous branch could be refactored into a separate function for better readability. - rctx_*() static functions could be rearranged so that their ordering in the source file better matches the order they are used in. However, I have not attempted to do that, so it might turn out to be too much work for too little gain. I am only signalling this because it is something I personally like to do. - "!is_answertarget_allowed" checks on lines 7017 and 7057 are misaligned: they should be in line with the column after the outer parenthesis, i.e. one column to the left from where they currently are. (This issue was also present in the code before refactoring.) - The "rdataset->type != fctx->type" check on line 7003 suffers from the same issue. - There is an extra space in "! dns_name_issubdomain(...)". (This issue was also present in the code before refactoring.) - Some vertical space could be reclaimed in rctx_any() by removing unnecessary line breaks. Phew, I think that's it, hope this helps!