Skip Menu |
Report information
The Basics
Id: 45362
Status: resolved
Priority: 50/50
Queue: bind9-public

People
Bug Information
Version Fixed: 9.12.0
Version Found: (no value)
Versions Affected: (no value)
Versions Planned: 9.12
Priority: P2 Normal
Severity: S2 Normal
CVSS Score: (no value)
CVE ID: (no value)
Component: BIND Server
Area: feature

Dates
Created:Fri, 09 Jun 2017 02:34:11 +0000
Updated:Fri, 04 Aug 2017 23:09:19 +0000
Closed:Fri, 04 Aug 2017 23:09:05 +0000



Subject: resolver.c refactoring
Trim down the lengths of resquery_response, answer_response, etc, for better readability and testability.
Download (untitled) / with headers
text/plain 9.2KiB
(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!
To: "Michal Kepien via RT" <bind9-public@isc.org>
From: "Evan Hunt" <each@isc.org>
Date: Wed, 2 Aug 2017 03:11:31 +0000
Subject: Re: [ISC-Bugs #45362] resolver.c refactoring
Download (untitled) / with headers
text/plain 5.4KiB
On Tue, Aug 01, 2017 at 09:05:41AM +0000, Michal Kepien via RT wrote: > 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(). Good catch. Those return values should have been set into rctx->result and ISC_R_COMPLETE returned instead. > I also noticed a glitch in BADCOOKIE handling which was already present > in the code before refactoring: Also a good catch, but I'm not sure about this being present before refactoring? I don't think there was a queryopts variable then, so query->options was the only place where we checked for DNS_FETCHOPT_TCP. For now I'm taking the approach of referencing and updating rctx->queryopts in all the rctx_* functions, but continuing to use query->options elsewhere. Possibly the right thing is simply to use rctx->query->options in all cases instead of rctx->queryopts, though... Hm. > 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. The acache was removed by Mukund's recent performance work; there's no point testing different behavior with and without it. As far as I can recall that's the only thing I was fixing in that particular commit. Sorry the message wasn't more detailed. > All that follows are purely suggestions/comments/bikeshedding. I've taken all your suggestions except as noted below. > 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. I think my reasoning was that rctx_respinit() would only ever be called once but rctx_initanswer() might be called more than once, and things that had been set would need to be cleared. (Though, I don't currently see a scenario where that happens.) In any case, the fields that are set in rctx_initanswer() are the ones that positively *must* be initialized at the beginning of rctx_answer() and rctx_noaanswer(), and I think I'd rather leave them there even if they're not strictly necessary. > 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? At last year's all-hands meeting we discussed the risks of this coding style -- it was the direct cause of a well-publicised Apple SSL bug -- and several of us felt that we should start moving away from it. So, in my refactoring work, both here and in query.c, I've been trying to remember to put brackets around single-line if statements in code that I touch. My feeling is that the code style is evolving on this point; we *can* leave single-line if statements unbracketed, but we don't have to and in this work I'm choosing not to. > o Is the "&& !rctx->chaining" part of the logical condition on line > 7225 necessary? Another really good catch. That mistake has been there since 2007. Fixed. (It's harmless, but we can also fix it in the non-refactored code if you think it's worth bothering. I suspect it gets optimized out by any decent compiler.) > 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. Yes, that's in my plans but hasn't been done yet. I plan to rearrange the functions into a more sensible order and write a long comment explaining how they relate to each other, too, like I did in query.c. Please review all the changes I've made in response to your other review comments. If they all look good, then I'll do the rearranging and documentation steps afterward, without changing any code. > - 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.) I often correct Mark's declensional quirk in printed documentation but I generally let him have his way in code comments. It's not just you, though, I find it weird too. :) > 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.) Nope, it doesn't make any sense. Probably it was meant to be somewhere else, but as this code dates back to 2002 (commit 5bd76af0) I doubt anyone would remember now what was intended. I'll remove it. > Phew, I think that's it, hope this helps! Very much so, thank you!
Download (untitled) / with headers
text/plain 12.1KiB
> > I also noticed a glitch in BADCOOKIE handling which was already present > > in the code before refactoring: > > Also a good catch, but I'm not sure about this being present before > refactoring? I don't think there was a queryopts variable then, so > query->options was the only place where we checked for DNS_FETCHOPT_TCP. I am not sure we are on the same page here, so please allow me to reiterate just in case: the bug is not about where DNS_FETCHOPT_TCP is _checked_, but rather where it is _set_. Before refactoring, a local variable called "options" was used for the same purpose for which rctx->queryopts is used after refactoring (line numbers taken from current master): 7582 static void 7583 resquery_response(isc_task_t *task, isc_event_t *event) { ... 7587 isc_boolean_t keep_trying, get_nameservers, resend, nextitem; 7597 unsigned int options; ... 7617 options = query->options; ... 7634 resend = ISC_FALSE; ... 8178 } else if (message->rcode == dns_rcode_badcookie && 8179 message->cc_ok) { 8180 /* 8181 * We have recorded the new cookie. 8182 */ 8183 if (BADCOOKIE(query->addrinfo)) 8184 query->options |= DNS_FETCHOPT_TCP; 8185 query->addrinfo->flags |= FCTX_ADDRINFO_BADCOOKIE; 8186 resend = ISC_TRUE; 8187 } else { ... 8194 } ... 8200 goto done; ... 8451 done: ... 8553 } else if (resend) { ... 8561 result = fctx_query(fctx, addrinfo, options); Thus, I still believe the bug was present in the code before, i.e. that it was not introduced by refactoring. (Perhaps this is worth fixing in a separate ticket?) > For now I'm taking the approach of referencing and updating rctx->queryopts > in all the rctx_* functions, but continuing to use query->options elsewhere. > > Possibly the right thing is simply to use rctx->query->options in all > cases instead of rctx->queryopts, though... Hm. The case I was worried about is that we would set a bit in rctx->queryopts and then later check for the same bit in rctx->queryopts, with the actual intention of checking whether that bit was set for the original query (i.e. in rctx->query->options). I could not find any code paths which do something like that (whenever rctx->queryopts is modified, rctx_done() is called almost immediately afterwards), but "enforcing" the "policy" I suggested ("always use rctx->query->options for checking bits, always use rctx->queryopts for setting bits") might prevent such code paths from being erroneously introduced in the future. While we are at it, how about renaming "queryopts" to "requeryopts" or something similar? It would make its purpose more explicit. > > 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. > > The acache was removed by Mukund's recent performance work; there's no > point testing different behavior with and without it. As far as I can > recall that's the only thing I was fixing in that particular commit. Sorry > the message wasn't more detailed. Ah, okay, thanks for the explanation. > > 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. > > I think my reasoning was that rctx_respinit() would only ever be called > once but rctx_initanswer() might be called more than once, and things > that had been set would need to be cleared. (Though, I don't currently > see a scenario where that happens.) Oh, okay, in that case, I would suggest to do one of the following: - rename rctx_initanswer() to rctx_resetanswer() so that what you wrote above is immediately clear and put a comment somewhere that such a design is intended, - since there is currently no scenario where rctx_initanswer() is called more than once for the same struct respctx, keeping things simple by trimming the explicit assignments down might be the simplest solution. > In any case, the fields that are set in rctx_initanswer() are the ones that > positively *must* be initialized at the beginning of rctx_answer() and > rctx_noaanswer(), and I think I'd rather leave them there even if they're > not strictly necessary. Sure, these are harmless right now. I am just worried that if the intended design of rctx_initanswer() allows it to be reused for the same struct respctx, selective assignments inside it may cause obscure bugs and/or unnecessary debugging efforts in the future [1]. > At last year's all-hands meeting we discussed the risks of this coding > style -- it was the direct cause of a well-publicised Apple SSL bug -- and > several of us felt that we should start moving away from it. So, in my > refactoring work, both here and in query.c, I've been trying to remember > to put brackets around single-line if statements in code that I touch. > > My feeling is that the code style is evolving on this point; we *can* > leave single-line if statements unbracketed, but we don't have to > and in this work I'm choosing not to. Fair enough, thanks for explaining the background. I am okay with both styles and I was just pointing out the inconsistency. > > o Is the "&& !rctx->chaining" part of the logical condition on line > > 7225 necessary? > > Another really good catch. That mistake has been there since 2007. > Fixed. (It's harmless, but we can also fix it in the non-refactored code > if you think it's worth bothering. I suspect it gets optimized out by any > decent compiler.) I would not bother. As you said, it is harmless. > > 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. > > Yes, that's in my plans but hasn't been done yet. I plan to rearrange the > functions into a more sensible order and write a long comment explaining > how they relate to each other, too, like I did in query.c. Ah, great, in that case, please carry on! > Please review all the changes I've made in response to your other review > comments. If they all look good, then I'll do the rearranging and > documentation steps afterward, without changing any code. I put down a few more suggestions below. > > - 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.) > > I often correct Mark's declensional quirk in printed documentation but > I generally let him have his way in code comments. It's not just you, > though, I find it weird too. :) Got it :) Some follow-up comments (line numbers taken from current head of rt45362, i.e. commit bb40559303): - I like defaulting to returning ISC_R_COMPLETE in rctx_parse(), but given that, could we please reorganize the ISC_R_UNEXPECTEDEND branch so that it looks like this: case ISC_R_UNEXPECTEDEND: if (fctx->rmessage->question_ok && (fctx->rmessage->flags & DNS_MESSAGEFLAG_TC) != 0 && (rctx->queryopts & DNS_FETCHOPT_TCP) == 0) { /* * We defer retrying via TCP for a bit so we can * check out this message further. */ rctx->truncated = ISC_TRUE; return (ISC_R_SUCCESS); } /* * Either the message ended prematurely, ... rctx_done(rctx, result); break; Right now, the break statement is inside the conditional branch and the case block ends with a return. IMHO it is legible, but confusing. - Consider this function: 7187 static isc_result_t 7188 rctx_scanauth(respctx_t *rctx) { 7189 fetchctx_t *fctx = rctx->fctx; 7190 isc_boolean_t done = ISC_FALSE; 7191 isc_result_t result; 7192 7193 result = dns_message_firstname(fctx->rmessage, DNS_SECTION_AUTHORITY); 7194 while (!done && result == ISC_R_SUCCESS) { ... 7247 result = dns_message_nextname(fctx->rmessage, 7248 DNS_SECTION_AUTHORITY); 7249 } 7250 7251 if (result == ISC_R_NOMORE) { 7252 result = ISC_R_SUCCESS; 7253 } 7254 7255 return (result); 7256 } Given that dns_message_firstname() and dns_message_nextname() can only return ISC_R_NOMORE or ISC_R_SUCCESS and the result variable is not assigned to anything else than the return value of these two functions, lines 7251-7255 can simply be replaced with: return (ISC_R_SUCCESS); - I believe you did not address this review comment, either in code or in your reply: > - 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. - rctx->glue_in_answer is now a boolean, so: - if ((rctx->glue_in_answer) != 0 && + if (rctx->glue_in_answer && - Misaligned continuation: 8244 inc_stats(fctx->res, 8245 dns_resstatscounter_edns0fail); - Closing parenthesis missing in comment: 6773 dns_rdatatype_t type; /* type being sought (set to 6774 * ANY if qtype was SIG or RRSIG */ [1] Cf. e.g. the last paragraph of this bug report: https://bugs.isc.org/Ticket/Display.html?id=45547#txn-483715
Download (untitled) / with headers
text/plain 2.6KiB
> - Consider this function: > > 7187 static isc_result_t > 7188 rctx_scanauth(respctx_t *rctx) { > 7189 fetchctx_t *fctx = rctx->fctx; > 7190 isc_boolean_t done = ISC_FALSE; > 7191 isc_result_t result; > 7192 > 7193 result = dns_message_firstname(fctx->rmessage, DNS_SECTION_AUTHORITY); > 7194 while (!done && result == ISC_R_SUCCESS) { > ... > 7247 result = dns_message_nextname(fctx->rmessage, > 7248 DNS_SECTION_AUTHORITY); > 7249 } > 7250 > 7251 if (result == ISC_R_NOMORE) { > 7252 result = ISC_R_SUCCESS; > 7253 } > 7254 > 7255 return (result); > 7256 } > > Given that dns_message_firstname() and dns_message_nextname() can > only return ISC_R_NOMORE or ISC_R_SUCCESS and the result variable is > not assigned to anything else than the return value of these two > functions, lines 7251-7255 can simply be replaced with: > > return (ISC_R_SUCCESS); In fact, we can go a step further: as this function always returns ISC_R_SUCCESS, there is no point in expecting any return value from it at all. How about making rctx_scanauth() return void and thus dropping the assignment to result on line 7315 and making rctx_answer() return ISC_R_SUCCESS instead of result on line 7326?
From: "Evan Hunt" <each@isc.org>
Subject: Re: [ISC-Bugs #45362] resolver.c refactoring
To: "Michal Kepien via RT" <bind9-public@isc.org>
Date: Wed, 2 Aug 2017 21:14:56 +0000
Download (untitled) / with headers
text/plain 2.7KiB
On Wed, Aug 02, 2017 at 08:42:23AM +0000, Michal Kepien via RT wrote: > Before refactoring, a local > variable called "options" was used for the same purpose for which > rctx->queryopts is used after refactoring (line numbers taken from > current master): Oh sorry, I see what you're talking about now. So, the options evolve as the query progresses: 1) options are passed to dns_resolver_createfetch(), which calls fctx_create() 2) fctx_create() sets fctx->options 3) fctx_try() calls fctx_query() with fctx->options 4) fctx_query() creates the query structure, sets query->options from the ones that were passed into it, and calls resquery_send() 5) resquery_response() creates the respctx structure, copying query->options to rctx->queryopts 6) if the resquery needs to be retried, rctx_resend() calls fctx_query() with rctx->queryopts, and we go back to step 4. When we update rctx->queryopts, then that becomes query->options the next time fctx_query() is called. If we update query->options directly, then that gets lost when we call rctx_resend(). (I know you already knew this, I'm just writing it down for my own clarity and for posterity.) > "enforcing" the "policy" I suggested ("always use > rctx->query->options for checking bits, always use rctx->queryopts for > setting bits") might prevent such code paths from being erroneously > introduced in the future. Okay, yes, this makes sense as a general rule, unless for some reason we want to update the behavior of the rctx functions during the processing of a response, as opposed to delaying the option changes until we retry. But I don't know of any place we need to do that currently. I've updated it as you suggested and will add comments about it when I document. > While we are at it, how about renaming "queryopts" to "requeryopts" or > something similar? It would make its purpose more explicit. "retryopts". > > I think my reasoning was that rctx_respinit() would only ever be called > > once but rctx_initanswer() might be called more than once, and things > > that had been set would need to be cleared. (Though, I don't currently > > see a scenario where that happens.) > > Oh, okay, in that case, I would suggest to do one of the following: Honestly, I think just adding comments will probably be more helpful here than changing names or removing initializations. > - I believe you did not address this review comment, either in code or > in your reply: > > > - 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. I figured if a terse and unclear comment has been there since 1999 and no one's done anything about it in all these years, it might as well go away. :)
I pushed one more fix and one more tweak. The former is hardly disputable because not applying it results in a compilation error when --enable-dnstap is used. As for the latter, I feel being explicit about the return value makes the code a bit more clear (and consistent with earlier returns in that function) and relieves the reader from having to scroll up in search of possible values of result. Feel free to disagree, though. I am happy with how the code looks now. I assume unit tests for all these elegant helper functions are coming along with comments, right? :)
Date: Fri, 4 Aug 2017 08:18:35 +0000
To: "Michal Kepien via RT" <bind9-public@isc.org>
Subject: Re: [ISC-Bugs #45362] resolver.c refactoring
From: "Evan Hunt" <each@isc.org>
CC:
As we discussed in jabber, the comments have now been pushed, and several of the rctx functions have been renamed for clarity. I didn't make any functional changes in that big commit (I promise) so if you just read the comments and they make sense, I think it's fine. An additional small functional change was made after that. Unit tests aren't coming now, but they'll be considerably more doable after this is committed and hopefully we'll get to them later.
Looks good to me. I pushed one more commit to fix a few typos and tweak formatting a bit.