Message ID | 20150220101303.GQ1594@spoyarek.pnq.redhat.com |
---|---|
State | New |
Headers | show |
On 02/20/2015 11:13 AM, Siddhesh Poyarekar wrote: > On Fri, Feb 20, 2015 at 09:10:41AM +0100, Florian Weimer wrote: >> Can we remove the logging altogether? Or at least for the >> RES_USE_DNSSEC case? >> >> The DO bit essentially means, “I'm fine with receiving unknown >> RR types”, it's not really related to DNSSEC. The reason for >> that is the fact that the DNSSEC protocol was changed twice (once >> for DNSSECbis, which is completely unrecognizable to the previous >> implementation, and once for NSEC3), and the flag was reused. >> >> So unless there is a compelling reason for logging this >> information, I'd say just remove it. > > Thanks for the context. I wasn't sure about removing the logging > altogether, but if it is going to be such a pain for DNSSEC, we > might as well silence it. How is this then? You are inconsistent about the space after p_class and p_type. I noticed there is an existing bug in p_type which affects the code: <https://sourceware.org/bugzilla/show_bug.cgi?id=18004> Your patch looks fine, but I suggest to be more explicit the comment, saying that DNSSEC uses many different types in responses which do not match the QTYPE.
On Fri, Feb 20, 2015 at 02:54:18PM +0100, Florian Weimer wrote: > You are inconsistent about the space after p_class and p_type. You mean between gethnamaddr.c and dns-host.c? I left that untouched because the gethnamaddr uses a completely different code formatting, likely from the bind source. > I noticed there is an existing bug in p_type which affects the code: > > <https://sourceware.org/bugzilla/show_bug.cgi?id=18004> > > Your patch looks fine, but I suggest to be more explicit the comment, > saying that DNSSEC uses many different types in responses which do not > match the QTYPE. Ack, I'll expand the comment. I'll wait for more comments and commit it on Tuesday since my Monday comes earlier than most people here. Siddhesh
On 02/20/2015 04:52 PM, Siddhesh Poyarekar wrote: > On Fri, Feb 20, 2015 at 02:54:18PM +0100, Florian Weimer wrote: >> You are inconsistent about the space after p_class and p_type. > > You mean between gethnamaddr.c and dns-host.c? I left that > untouched because the gethnamaddr uses a completely different code > formatting, likely from the bind source. Oh, I suspected as much. Fine then.
On 02/20/2015 05:13 AM, Siddhesh Poyarekar wrote: > Thanks for the context. I wasn't sure about removing the logging > altogether, but if it is going to be such a pain for DNSSEC, we might > as well silence it. How is this then? > > Siddhesh > > [BZ #14841] > * resolv/gethnamaddr.c (getanswer): Skip logging if > RES_USE_DNSSEC is set. > * resolv/nss_dns/dns-host.c (getanswer_r): Likewise. Looks good to me, and simplifies the code. I guess given the churn we're going to have to wait a while before this standardizes to be able to provide useful logging. Cheers, Carlos.
diff --git a/resolv/gethnamaddr.c b/resolv/gethnamaddr.c index a861a84..0fe2ad9 100644 --- a/resolv/gethnamaddr.c +++ b/resolv/gethnamaddr.c @@ -331,23 +331,16 @@ getanswer (const querybuf *answer, int anslen, const char *qname, int qtype) buflen -= n; continue; } - if ((type == T_SIG) || (type == T_KEY) || (type == T_NXT)) { - /* We don't support DNSSEC yet. For now, ignore - * the record and send a low priority message - * to syslog. - */ - syslog(LOG_DEBUG|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; - } if (type != qtype) { - syslog(LOG_NOTICE|LOG_AUTH, + /* Log a low priority message if we get an unexpected + * record, but skip it if we are using DNSSEC. + */ + if ((_res.options & RES_USE_DNSSEC) == 0) { + 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 f715ab0..5afc955 100644 --- a/resolv/nss_dns/dns-host.c +++ b/resolv/nss_dns/dns-host.c @@ -820,26 +820,18 @@ getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype, linebuflen -= n; continue; } - if (__builtin_expect (type == T_SIG, 0) - || __builtin_expect (type == T_KEY, 0) - || __builtin_expect (type == T_NXT, 0)) - { - /* We don't support DNSSEC yet. For now, ignore the record - and send a low priority message to syslog. */ - syslog (LOG_DEBUG | 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; - } if (type == T_A && qtype == T_AAAA && map) 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)); + /* Log a low priority message if we get an unexpected record, but + skip it if we are using DNSSEC. */ + if ((_res.options & RES_USE_DNSSEC) == 0) + 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++ ? */ }