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. :)