Delivered-To: bind9-public@bugs.isc.org X-RT-Original-Encoding: utf-8 From: "Evan Hunt" User-Agent: Mutt/1.5.23 (2014-03-12) X-RT-Incoming-Encryption: Not encrypted In-Reply-To: content-type: text/plain; charset="utf-8" Content-Disposition: inline X-RT-Interface: Email MIME-Version: 1.0 Return-Path: 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 867B4D78AFB for ; Wed, 2 Aug 2017 21:14:56 +0000 (UTC) Received: by bikeshed.isc.org (Postfix, from userid 10292) id 431C5216C1E; Wed, 2 Aug 2017 21:14:56 +0000 (UTC) References: <20170802031131.GA63960@isc.org> X-Original-To: bind9-public@bugs.isc.org Message-ID: <20170802211456.GD72046@isc.org> Subject: Re: [ISC-Bugs #45362] resolver.c refactoring To: "Michal Kepien via RT" Date: Wed, 2 Aug 2017 21:14:56 +0000 From each@isc.org Wed Aug 2 21:14:56 2017 RT-Message-ID: Content-Length: 2817 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. :)