Re: [PATCH] Silence resolver logging for DNAME records when DNSSEC is enabled

["Carlos O'Donell" <carlos at redhat dot com>]

On 02/19/2015 02:05 PM, Siddhesh Poyarekar wrote:
> DNAME records are a convenient way to set up RRSIG for an entire
> subtree of a domain name tree instead of signing each of those
> records.  Querying on such domains result in messages about a mismatch
> in the query type and returned record type.  This patch disables the
> logging of this message for DNAME records if the DO bit is set.

Makes perfect sense.

> Tested on x86_64.
> 
> 	* resolv/gethnamaddr.c (getanswer): Don't log about record
> 	type mismatch for DNAME if DNSSEC is requested.
> 	* resolv/nss_dns/dns-host.c (getanswer_r): Likewise.

One nit in terms of readability.

If you disagree with me, feel free to commit as is.

> ---
>  resolv/gethnamaddr.c      | 14 +++++++++++---
>  resolv/nss_dns/dns-host.c | 11 ++++++++---
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/resolv/gethnamaddr.c b/resolv/gethnamaddr.c
> index 9e0c498..ae55fac 100644
> --- a/resolv/gethnamaddr.c
> +++ b/resolv/gethnamaddr.c
> @@ -349,10 +349,18 @@ getanswer (const querybuf *answer, int anslen, const char *qname, int qtype)
>  			continue;
>  		}
>  		if (type != qtype) {
> -			syslog(LOG_NOTICE|LOG_AUTH,
> +			/* Skip logging if we received a DNAME when we have set
> +			 * the DO bit.  DNAME records are a convenient way to
> +			 * set up DNSSEC records and such setups can make this
> +			 * log message needlessly noisy.
> +			 */
> +			if ((_res.options & RES_USE_DNSSEC) == 0
> +			    || type != T_DNAME) {

I find it clearer to write:

if (!((_res.options & RES_USE_DNSSEC)
      && type == T_DNAME)) {

i.e. invert the boolean to get an easier to understand "&&".

Which reads straightforwardly as:

If you request DNSSEC and you got a T_DNAME record type, don't log it.

While the present conditional reads as:

If it's not a T_DNAME, log it.
If we didn't request DNSSEC, log it.
Implies: If we requested DNSSEC, and it is a T_DNAME, don't log it.

> +				syslog(LOG_NOTICE|LOG_AUTH,
>  	       "gethostby*.getanswer: asked for \"%s %s %s\", got type \"%s\"",
> -			       qname, p_class(C_IN), p_type(qtype),
> -			       p_type(type));
> +					qname, p_class(C_IN), p_type(qtype),
> +					p_type(type));
> +			}
>  			cp += n;
>  			continue;		/* XXX - had_error++ ? */
>  		}
> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index b10c94e..510d388 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -844,9 +844,14 @@ getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype,
>  	have_to_map = 1;
>        else if (__glibc_unlikely (type != qtype))
>  	{
> -	  syslog (LOG_NOTICE | LOG_AUTH,
> -	       "gethostby*.getanswer: asked for \"%s %s %s\", got type \"%s\"",
> -		  qname, p_class (C_IN), p_type (qtype), p_type (type));
> +	  /* Skip logging if we received a DNAME when we have set the DO bit.
> +	     DNAME records are a convenient way to set up DNSSEC records and
> +	     such setups can make this log message needlessly noisy.  */
> +	  if ((_res.options & RES_USE_DNSSEC) == 0 || type != T_DNAME)

Likewise with the conditional.

Otherwise it's fine.

> +	    syslog (LOG_NOTICE | LOG_AUTH,
> +		    "gethostby*.getanswer: asked for \"%s %s %s\", "
> +		    "got type \"%s\"",
> +		    qname, p_class (C_IN), p_type (qtype), p_type (type));
>  	  cp += n;
>  	  continue;			/* XXX - had_error++ ? */
>  	}
> 

Cheers,
Carlos.