X-RT-Incoming-Encryption: Not encrypted X-RT-Interface: Email MIME-Version: 1.0 Received: from bikeshed.isc.org (bikeshed.isc.org [149.20.48.19]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mail.isc.org", Issuer "RapidSSL CA" (not verified)) by bugs.isc.org (Postfix) with ESMTPS id 26351D78AFB for ; Wed, 2 Aug 2017 03:11:33 +0000 (UTC) Received: by bikeshed.isc.org (Postfix, from userid 10292) id 9C5C1216C1E; Wed, 2 Aug 2017 03:11:31 +0000 (UTC) Content-Disposition: inline To: "Michal Kepien via RT" References: Delivered-To: bind9-public@bugs.isc.org X-RT-Original-Encoding: utf-8 Message-ID: <20170802031131.GA63960@isc.org> From: "Evan Hunt" User-Agent: Mutt/1.5.23 (2014-03-12) Date: Wed, 2 Aug 2017 03:11:31 +0000 X-Original-To: bind9-public@bugs.isc.org From each@isc.org Wed Aug 2 03:11:33 2017 Return-Path: Subject: Re: [ISC-Bugs #45362] resolver.c refactoring content-type: text/plain; charset="utf-8" In-Reply-To: RT-Message-ID: Content-Length: 5575 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!