BIND 10 #2155: Collect query/response statistics items in Auth module
BIND 10 Development
do-not-reply at isc.org
Fri Oct 12 07:33:52 UTC 2012
#2155: Collect query/response statistics items in Auth module
-------------------------------------+-------------------------------------
Reporter: | Owner: vorner
y-aharen | Status: reviewing
Type: | Milestone:
enhancement | Sprint-20121023
Priority: | Resolution:
medium | Sensitive: 0
Component: | Sub-Project: DNS
b10-auth | Estimated Difficulty: 5
Keywords: | Total Hours: 0
Defect Severity: N/A |
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by y-aharen):
* owner: y-aharen => vorner
Comment:
Hello,
Thank you for reviewing.
Replying to [comment:13 vorner]:
> The headers in EXTRA_DIST look unusual. I know it may be reasonable
solution, since there's no compiled code and no library, but maybe a
comment to the makefile would be good.
OK, I added some notes.
> The note about SIG(0) still applies:
> {{{
> // statistics: check TSIG attributes
> // SIG(0) is currently not implemented in Auth
> }}}
>
> It seems the SIG(0) is some kind of signing algorithm or something? But
if so, it is unclear how it relates to the surrounding code and why it is
important for the statistics code that it is not implemented in Auth.
For the detail of SIG(0), please see RFC 2931.
Per-RRtype counter for SIG(0) is defined in BIND 9. I included it for BIND
9 compatibility.
> Also, what is the reason for this bit of code having a separate block
around it?
> {{{#!c++
> // note: This can only be reliable after TSIG check
succeeds.
> {
> ConstEDNSPtr edns = message.getEDNS();
> if (edns != NULL) {
> stats_attrs.setQueryEDNS(true, edns->getVersion() == 0);
> stats_attrs.setQueryDO(edns->getDNSSECAwareness());
> }
> }
> }}}
I've put them into the block to indicate edns is used only for statistics.
If it is unclear and meaningless, I'll remove the surrounding braces.
> In this bit of code, how is it possible for the qptr to be NULL? If it
is NULL, what does that mean? And if it can't, should we assert for it
instead?
> {{{#!c++
> const QuestionIterator qiter = response.beginQuestion();
> if (qiter != response.endQuestion()) {
> // get the first and only question section
> const QuestionPtr qptr = *qiter;
> if (qptr != NULL) {
> }}}
Yes, it can't be NULL, I've removed the meaningless NULL check.
> In this code, should we distinguish NXDOMAIN as well as NXRRSET?
> {{{#!c++
> if (rcode == Rcode::NOERROR_CODE) {
> if (answer_rrs > 0) {
> // QrySuccess
> server_qr_counter_.inc(QR_QRYSUCCESS);
> } else {
> if (is_aa_set) {
> // QryNxrrset
> server_qr_counter_.inc(QR_QRYNXRRSET);
> } else {
> // QryReferral
> server_qr_counter_.inc(QR_QRYREFERRAL);
> }
> }
> } else if (rcode == Rcode::REFUSED_CODE) {
> // AuthRej
> server_qr_counter_.inc(QR_QRYREJECT);
> }
> }}}
QR_QRYNXRRSET is not for Rcode. It's for the item qrynxrrrset
(server/nsstat/QryNxrrset in BIND 9).
NXDomain (Rcode = 3) and NXRRSet (Rcode = 8) counters are incremented in
another place.
> There are magic constants in the code. Maybe they should be defined as
constants somewhere. Also, the comments like „qtype 0..257“ are really
useless. They say the obvious thing about the code, exactly what is
written there, but don't clarify anything about the reason for the code or
such.:
> {{{#!c++
> if (qtype < 258) {
> // qtype 0..257
> qtype_type = QRQTypeToQRCounterType[qtype];
> } else if (qtype < 32768) {
> // qtype 258..32767
> qtype_type = QR_QTYPE_OTHER;
> } else if (qtype < 32770) {
> // qtype 32768..32769
> qtype_type = QR_QTYPE_TA + (qtype - 32768);
> } else {
> // qtype 32770..65535
> qtype_type = QR_QTYPE_OTHER;
> }
> }}}
>
> {{{#!c++
> if (rcode < 23) {
> // rcode 0..22
> }}}
>
> I smell something bad if I see code like this. This is very long and
easy to make the number of lines wrong in one way or another. And, when
you do it wrong, nothing breaks, you just get wrong answers:
> {{{#!c++
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> QR_QTYPE_OTHER,
> }}}
>
> It would be better to build the array from bunch of position-value pairs
(omitting the OTHER ones, that'd be there in case no pair would have that
position). Or even better, just from the list of „special“ types (like A
and SOA and so on) and pull the positions from the dns++ library.
I added some notes not to make a mistake on editing the list. The latter
option is not applicable easily because there're some types that are not
in dns++.
> The test functions `checkAllRcodeCountersZeroExcept` and
`checkAllRcodeCountersZero` look very similar. Would it be possible to
share the code between them somehow? Maybe implement the
`checkAllRcodeCountersZero` as a call to `checkAllRcodeCountersZero` with
some random RCode and 0?
Yes, I modified as you suggested.
> I don't understand the purpose of this function. What does it do? Maybe
comment it?
> {{{#!c++
> void
> expectCounterItem(ConstElementPtr stats,
> const std::string& item, const int expected) {
> ConstElementPtr value(Element::create(0));
> if (item == "queries.udp" || item == "queries.tcp" || expected != 0)
{
> ASSERT_TRUE(stats->find(item, value)) << " Item: " << item;
> value = stats->find(item);
> EXPECT_EQ(expected, value->intValue()) << " Item: " << item;
> } else {
> ASSERT_FALSE(stats->find(item, value)) << " Item: " << item
<<
> std::endl << " Value: " << value->intValue();
> }
> }
> }}}
I added some notes.
> This comment seems wrong, `request.tcp` is both in incremented and not
incremented. Should the first line say `request.udp`?
> {{{#!c++
> // After processing the UDP query, these counters should be incremented:
> // request.tcp, opcode.query, qtype.ns, rcode.refused, response
> // and these counters should not be incremented:
> // request.tcp
> }}}
That was wrong, I fixed it.
> Also, the same test as above and most of the similar check one value
less than what is being listed in the comments. Is it forgotten, or is it
some trick I don't see?
>
> May I suggest removing useless code, like empty constructors and
destructors?
> {{{#!c++
> CountersTest() : counters() {}
> ~CountersTest() {}
> }}}
>
> {{{#!c++
> Counters::~Counters() {}
> }}}
>
> {{{#!c++
> inline QRAttributes::~QRAttributes()
> {}
> }}}
>
> {{{#!c++
> inline Counter::~Counter() {}
> }}}
>
> {{{#!c++
> inline ~ConstIterator() {}
> }}}
>
> {{{#!c++
> inline CounterDictionary::~CounterDictionary() {}
> }}}
After removing some empty destructors, compiler complains that something
is wrong. I have no idea to remove them safely, I'll leave them.
> Also, the canonical way to inline methods is not the `inline` keyword,
but having them inside the class definition itself, like this:
> {{{#!c++
> class Class {
> public:
> void thisMethodGetsInlined() {
> ...
> }
> };
> }}}
OK, modified as figured.
> Does this function return false every time? Is there any use of the
return value then?
> {{{#!c++
> bool
> checkCountersAllZeroExcept(const isc::data::ConstElementPtr counters,
> const std::set<std::string>& except_for) {
> }}}
No, the return value is useless, it should be void. I fixed it.
> The constructor and `reset` method of `QRAttributes` look very similar.
Would it be better to just call reset from the constructor?
Yes, they works similarly. I referred to isc::auth::Query class, which
also has explicit constructor and reset() member function. If it's not a
good example, I'll fix it.
> I think these are auto-generated by compiler when needed:
> {{{#!c++
> inline ConstIterator& operator=(const ConstIterator& source) {
> iterator_ = source.iterator_;
> return (*this);
> }
>
> inline ConstIterator(const ConstIterator& source) :
> iterator_(source.iterator_)
> {}
> }}}
OK, I've removed them.
--
Ticket URL: <http://bind10.isc.org/ticket/2155#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list