Opened 8 years ago

Closed 7 years ago

#2155 closed enhancement (complete)

Collect query/response statistics items in Auth module

Reported by: y-aharen Owned by: y-aharen
Priority: medium Milestone: Sprint-20121106
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 10
Total Hours: 6.53 Internal?: no

Description (last modified by y-aharen)

Collect query/response statistics items (e.g. opcode, qtype, edns0) in Auth module, and increment counters.

Modify query processing functions (mainly AuthSrv::processMessage) to collect attributes of a query/response. Add a functionality to increment counters corresponding to the query/response.

This ticket depends on #2154.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by y-aharen

  • Owner set to y-aharen
  • Status changed from new to accepted

comment:2 Changed 8 years ago by shane

  • Milestone changed from New Tasks to StatsRedesign

comment:3 Changed 8 years ago by y-aharen

  • Owner changed from y-aharen to UnAssigned
  • Status changed from accepted to reviewing

The branch is ready for reviewing. Please also refer to branch 'stats201209-auth-merge'.

comment:4 Changed 8 years ago by y-aharen

  • Description modified (diff)

comment:5 Changed 8 years ago by jelte

  • Milestone changed from StatsRedesign to Sprint-20120904

comment:6 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 Changed 8 years ago by vorner

  • Owner changed from vorner to y-aharen

Hello

So, from the most serious. It does not compile for me:

/bin/sed -e "s|@@LOCALSTATEDIR@@|/home/vorner/testing/bind10/var|" auth.spec.pre >auth.spec
../../../src/bin/auth/statistics_items.h:75:13: error: ‘{anonymous}::SocketCounterItemName’ defined but not used [-Werror=unused-variable]
../../../src/bin/auth/statistics_items.h:250:13: error: ‘{anonymous}::QRCounterItemName’ defined but not used [-Werror=unused-variable]
cc1plus: all warnings being treated as errors
make[6]: *** [statistics.o] Error 1
make[6]: *** Waiting for unfinished jobs....
make[6]: Leaving directory `/home/vorner/work/bind10/src/bin/auth'

Also, distcheck does not pass, for a different reason (is the new statistics_items.h in AUTH_SOURCES in the makefile? ):

g++ -DHAVE_CONFIG_H -I. -I../../../../src/bin/auth -I../../..  -I../../../../src/lib -I../../../src/lib -I../../../../src/bin -I../../../src/bin -I../../../../src/lib/dns -I../../../src/lib/dns -I../../../../src/lib/cc -I../../../src/lib/cc -I../../../../src/lib/asiolink -I../../../src/lib/asiolink  -DOS_LINUX  -I../../../../ext/asio -I../../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O2 -MT statistics.o -MD -MP -MF .deps/statistics.Tpo -c -o statistics.o ../../../../src/bin/auth/statistics.cc
../../../../src/bin/auth/statistics.cc:16:35: fatal error: auth/statistics_items.h: No such file or directory
compilation terminated.
make[7]: *** [statistics.o] Error 1
make[7]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20120712/_build/src/bin/auth'

I also must agree with jelte and jinmei that the conversion from opcodes to the enum by long if-else if-else if-… sequence is rather ugly, for following reasons:

  • It is long code that is not easy to read (the branches are almost the same, but they differ slightly, making the concentration drift away while reading them and miss important things, like that this one has + 1 somewhere in it).
  • It is fragile. It depends crucially on the order of the enum. If someone changes the order of the enum (by accident, or something), odd things like increments of completely unrelated counters can happen.
  • As jinmei noted, it may be performance problem.
  • It is not needed. What it does is saving few bytes by compacting some opcodes together, but that is very small amount.

So I propose this:

  • Have several large enough arrays (one for RRType, one for the opcodes, etc).
  • Index by the values from DNS directly. This is faster and much simpler to read.
  • If the compression of multiple opcodes or other things is needed, do it during the report generating (when sending the statisticts). This is much less performance sensitive place and can be actually coded in a simpler way by defining constants of names and indices in the array to sum together, like this (more like a pseudo-code than really valid C++ and the numbers are made up a little, but it's the idea that matters):
    static Definition definitions[] = {
            {"QUERY", {0, 1, 2}},
            {"NOTIFY", {4, 5}},
            {"OTHER", {3}}
    };
    
    
    for definition(definitions) {
            unsigned sum = 0;
            for i (definition.indices) {
                    sum += counter_array[i];
            }
            if (definition.name == "OTHER") {
                    for i (6..) {
                            sum += counter_array[i];
                    }
            }
            result.push(definition.name, sum);
    }
    
  • Also, we would be able to provide detailed statistics that are not compressed, if asked for, just by providing the raw data of the array.

Aside from these, there are several style and comment issues:

The resumeServer method header:

  • I think you used wrong verb, I have no idea what you meant.

{{{c++

/ \param stats_attrs query/response attributes for statistics which is
/ not explained in \p messsage

}}}

  • Also, the fact that the stats_attrs are passed as reference and modified as a side effect is somewhat counter-intuitive (as it is an input parameter) and ugly. If you come up with no better idea than that, it would deserve a comment explaining the odd fact and the reasons for it.

This bit of code:

// statistics: check TSIG attributes
// SIG(0) is currently not implemented in Auth
stats_attrs.setQuerySig(true, false,
                        ( tsig_error == TSIGError::NOERROR() ));
  • What is the thing with SIG(0)? Is it my lack of knowledge about DNS that it makes no sense to me?
  • The parentheses around the == comparison are both unneeded and wrongly placed according to our style (in our style, they would have no spaces inside them). Maybe remove them completely?

Why are there the extra braces around the EDNS thing?

        // statistics: check EDNS
        //     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());
            }
        }

        // statistics: check OpCode
        //     note: This can only be reliable after TSIG check succeeds.
        stats_attrs.setQueryOpCode(opcode.getCode());

The answerHasSent looks as wrong in English. Maybe answerWasSent or answerIsSent?

Spacing in void registerStatisticsValidator (Counters::validator_type validator); is wrong.

When looking at this code, it looks like it is missing UDP. I know the other parts compute it when needed, but a comment here that it is not forgotten, but will be computed, would be nice.

CountersImpl::inc(const QRAttributes& qrattrs, const Message& response) {
    // protocols carrying request
    if (qrattrs.req_ip_version_ == AF_INET) {
        server_qr_counter_.inc(QR_REQUEST_IPV4);
    } else if (qrattrs.req_ip_version_ == AF_INET6) {
        server_qr_counter_.inc(QR_REQUEST_IPV6);
    }
    if (qrattrs.req_transport_protocol_ == IPPROTO_TCP) {
        server_qr_counter_.inc(QR_REQUEST_TCP);
    }

Why is this code commented out? Is it deprecated? If so, shouldn't it be deleted completely? If it is just broken, a comment with FIXME or TODO mark would be in place:

CountersImpl::inc(const QRAttributes& qrattrs, const Message& response) {
    // protocols carrying request
    if (qrattrs.req_ip_version_ == AF_INET) {
        server_qr_counter_.inc(QR_REQUEST_IPV4);
    } else if (qrattrs.req_ip_version_ == AF_INET6) {
        server_qr_counter_.inc(QR_REQUEST_IPV6);
    }
    if (qrattrs.req_transport_protocol_ == IPPROTO_TCP) {
        server_qr_counter_.inc(QR_REQUEST_TCP);
    }

The doxygen comment for the QRAttributes class should be outside the class, not inside ‒ this documents the friend declaration now.

The setOrigin method actually can throw an allocation exception, because it assigns std::string object.

Again, commented-out code in the tests:

#endif

#if 0
// DISABLED: these interfaces, namely,
// Counters.inc(const Counters::ServerCounterType&) and
// Counters.inc(const isc::dns::Opcode&) and
// Counters.inc(const isc::dns::Rcode&) has been removed.

If the interfaces were removed completely, should the tests be just removed? Otherwise, for how long will the tests be there?

Also, it looks slightly strange to end one if(0) and then start it again right away ‒ wouldn't it be enough to have just one big (provided there's a reason to leave them there)?

comment:8 Changed 8 years ago by jinmei

  • Owner changed from y-aharen to jinmei
  • Status changed from reviewing to accepted

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to y-aharen
  • Status changed from accepted to assigned

comment:10 Changed 8 years ago by y-aharen

  • Status changed from assigned to accepted

comment:11 Changed 7 years ago by y-aharen

  • Owner changed from y-aharen to UnAssigned
  • Status changed from accepted to reviewing

branch 2155_2 is ready for reviewing.

comment:12 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:13 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to y-aharen

Hello

I'm glad the biggest issue was sorted out. However, some more issues still remain or new ones appear.

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.

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.

Also, what is the reason for this bit of code having a separate block around it?

        //     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());
            }
        }

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?

    const QuestionIterator qiter = response.beginQuestion();
    if (qiter != response.endQuestion()) {
        // get the first and only question section
        const QuestionPtr qptr = *qiter;
        if (qptr != NULL) {

In this code, should we distinguish NXDOMAIN as well as NXRRSET?

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);
}

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

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;
}
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:

    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.

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?

I don't understand the purpose of this function. What does it do? Maybe comment it?

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();
    }
}

This comment seems wrong, request.tcp is both in incremented and not incremented. Should the first line say request.udp?

// 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

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?

    CountersTest() : counters() {}
    ~CountersTest() {}
Counters::~Counters() {}
inline QRAttributes::~QRAttributes()
{}
inline Counter::~Counter() {}
inline ~ConstIterator() {}
inline CounterDictionary::~CounterDictionary() {}

Also, the canonical way to inline methods is not the inline keyword, but having them inside the class definition itself, like this:

class Class {
public:
        void thisMethodGetsInlined() {
                ...
        }
};

Does this function return false every time? Is there any use of the return value then?

bool
checkCountersAllZeroExcept(const isc::data::ConstElementPtr counters,
                           const std::set<std::string>& except_for) {

The constructor and reset method of QRAttributes look very similar. Would it be better to just call reset from the constructor?

I think these are auto-generated by compiler when needed:

inline ConstIterator& operator=(const ConstIterator& source) {
    iterator_ = source.iterator_;
    return (*this);
}

inline ConstIterator(const ConstIterator& source) :
    iterator_(source.iterator_)
{}

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by y-aharen

  • Owner changed from y-aharen to vorner

Hello,

Thank you for reviewing.

Replying to 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?

        //     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?

    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?

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

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;
}
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:

    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?

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?

// 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?

    CountersTest() : counters() {}
    ~CountersTest() {}
Counters::~Counters() {}
inline QRAttributes::~QRAttributes()
{}
inline Counter::~Counter() {}
inline ~ConstIterator() {}
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:

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?

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:

inline ConstIterator& operator=(const ConstIterator& source) {
    iterator_ = source.iterator_;
    return (*this);
}

inline ConstIterator(const ConstIterator& source) :
    iterator_(source.iterator_)
{}

OK, I've removed them.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to y-aharen

Hello

Replying to y-aharen:

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.

Can you add the note about compatibility with bind9, then, into the comment?

Also, what is the reason for this bit of code having a separate block around it?

        //     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.

A comment saying so may be better, this really is not obvious what it means.

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:

I still don't think it is optimal solution, but the comments with numbers should be good enough.

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?

What about this comment? I didn't find it addressed anywhere.

May I suggest removing useless code, like empty constructors and destructors?

    CountersTest() : counters() {}
    ~CountersTest() {}
Counters::~Counters() {}
inline QRAttributes::~QRAttributes()
{}
inline Counter::~Counter() {}
inline ~ConstIterator() {}
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.

Could you paste the error, please? I'd be interested to know what is happening there, because it seems it shouldn't be happening.

Also, the canonical way to inline methods is not the inline keyword, but having them inside the class definition itself, like this:

OK, modified as figured.

The inline keyword is not actually needed when the method is inside the class definition, it is implicit. That's why I suggested it ‒ the inline keywords look like waste of space O:-).

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 don't know. On one side, the construction with initializers might be slightly faster than calling reset(). But I'd guess the compiler would optimise it anyway.

But what I don't like on this approach is the duplication of code. Once a new item is added there, it needs to be set to the default value at two places and one may forget to update one of them. So I personally prefer to reuse the code of reset() by calling it from the constructor, but it may be a subjective preference.

Thank you

comment:16 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by y-aharen

Hello,

Sorry for late response.

Replying to vorner:

Hello

Replying to y-aharen:

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.

Can you add the note about compatibility with bind9, then, into the comment?

OK, I added some notes.

Also, what is the reason for this bit of code having a separate block around it?

        //     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.

A comment saying so may be better, this really is not obvious what it means.

I removed surrounding braces.

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?

What about this comment? I didn't find it addressed anywhere.

Sorry I missed to solve it. In #2155 all of the items are collected, but only limited items are passed to Stats. The comments were incorrect.

May I suggest removing useless code, like empty constructors and destructors?

    CountersTest() : counters() {}
    ~CountersTest() {}
Counters::~Counters() {}
inline QRAttributes::~QRAttributes()
{}
inline Counter::~Counter() {}
inline ~ConstIterator() {}
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.

Could you paste the error, please? I'd be interested to know what is happening there, because it seems it shouldn't be happening.

When I removed Counters::~Counters() from statistics.h and statistics.cc, g++ complains:

  CXX    auth_srv.o
In file included from /usr/include/boost/smart_ptr/scoped_ptr.hpp:15,
                 from /usr/include/boost/scoped_ptr.hpp:14,
                 from ../../../ext/asio/asio/system_error.hpp:19,
                 from ../../../ext/asio/asio/detail/impl/throw_error.ipp:21,
                 from ../../../ext/asio/asio/detail/throw_error.hpp:50,
                 from ../../../ext/asio/asio/ip/impl/address_v4.hpp:20,
                 from ../../../ext/asio/asio/ip/address_v4.hpp:205,
                 from ../../../ext/asio/asio/ip/address.hpp:21,
                 from ../../../src/lib/asiolink/io_address.h:23,
                 from ../../../src/lib/asiolink/io_endpoint.h:26,
                 from ../../../src/lib/asiolink/io_message.h:28,
                 from ../../../src/lib/asiolink/simple_callback.h:18,
                 from ../../../src/lib/asiolink/asiolink.h:23,
                 from auth_srv.cc:19:
/usr/include/boost/checked_delete.hpp: In function ‘void boost::checked_delete(T*) [with T = isc::auth::statistics::CountersImpl]’:
/usr/include/boost/smart_ptr/scoped_ptr.hpp:80:   instantiated from ‘boost::scoped_ptr<T>::~scoped_ptr() [with T = isc::auth::statistics::CountersImpl]’
../../../src/bin/auth/statistics.h:161:   instantiated from here
/usr/include/boost/checked_delete.hpp:32: error: invalid application of ‘sizeof’ to incomplete type ‘isc::auth::statistics::CountersImpl’
/usr/include/boost/checked_delete.hpp:32: error: creating array with negative size (‘-0x00000000000000001’)

Also, the canonical way to inline methods is not the inline keyword, but having them inside the class definition itself, like this:

OK, modified as figured.

The inline keyword is not actually needed when the method is inside the class definition, it is implicit. That's why I suggested it ‒ the inline keywords look like waste of space O:-).

Sorry, I misunderstood it. I removed inline keyword.

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 don't know. On one side, the construction with initializers might be slightly faster than calling reset(). But I'd guess the compiler would optimise it anyway.

But what I don't like on this approach is the duplication of code. Once a new item is added there, it needs to be set to the default value at two places and one may forget to update one of them. So I personally prefer to reuse the code of reset() by calling it from the constructor, but it may be a subjective preference.

OK, I modified the code as you suggested.

Thanks,

comment:17 Changed 7 years ago by y-aharen

  • Owner changed from y-aharen to vorner

comment:18 in reply to: ↑ 16 Changed 7 years ago by vorner

  • Owner changed from vorner to y-aharen
  • Total Hours changed from 0 to 6.53

Hello

Replying to y-aharen:

Could you paste the error, please? I'd be interested to know what is happening there, because it seems it shouldn't be happening.

When I removed Counters::~Counters() from statistics.h and statistics.cc, g++ complains:

  CXX    auth_srv.o
In file included from /usr/include/boost/smart_ptr/scoped_ptr.hpp:15,
                 from /usr/include/boost/scoped_ptr.hpp:14,
                 from ../../../ext/asio/asio/system_error.hpp:19,
                 from ../../../ext/asio/asio/detail/impl/throw_error.ipp:21,
                 from ../../../ext/asio/asio/detail/throw_error.hpp:50,
                 from ../../../ext/asio/asio/ip/impl/address_v4.hpp:20,
                 from ../../../ext/asio/asio/ip/address_v4.hpp:205,
                 from ../../../ext/asio/asio/ip/address.hpp:21,
                 from ../../../src/lib/asiolink/io_address.h:23,
                 from ../../../src/lib/asiolink/io_endpoint.h:26,
                 from ../../../src/lib/asiolink/io_message.h:28,
                 from ../../../src/lib/asiolink/simple_callback.h:18,
                 from ../../../src/lib/asiolink/asiolink.h:23,
                 from auth_srv.cc:19:
/usr/include/boost/checked_delete.hpp: In function ‘void boost::checked_delete(T*) [with T = isc::auth::statistics::CountersImpl]’:
/usr/include/boost/smart_ptr/scoped_ptr.hpp:80:   instantiated from ‘boost::scoped_ptr<T>::~scoped_ptr() [with T = isc::auth::statistics::CountersImpl]’
../../../src/bin/auth/statistics.h:161:   instantiated from here
/usr/include/boost/checked_delete.hpp:32: error: invalid application of ‘sizeof’ to incomplete type ‘isc::auth::statistics::CountersImpl’
/usr/include/boost/checked_delete.hpp:32: error: creating array with negative size (‘-0x00000000000000001’)

That is indeed very strange. It doesn't seem to be related at all. Anyway, if it helps, let's leave it this way.

It looks OK to merge.

Thank you

comment:19 Changed 7 years ago by y-aharen

  • Add Hours to Ticket changed from 0 to 10
  • Resolution set to complete
  • Status changed from reviewing to closed

make check, make lettuce, make systest passed. I merged the branch into master.
Thank you for reviewing.

Note: See TracTickets for help on using tickets.