BIND 10 #2155: Collect query/response statistics items in Auth module
BIND 10 Development
do-not-reply at isc.org
Thu Oct 25 12:10:51 UTC 2012
#2155: Collect query/response statistics items in Auth module
-------------------------------------+-------------------------------------
Reporter: | Owner: y-aharen
y-aharen | Status: reviewing
Type: | Milestone:
enhancement | Sprint-20121106
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 |
-------------------------------------+-------------------------------------
Comment (by y-aharen):
Hello,
Sorry for late response.
Replying to [comment:15 vorner]:
> Hello
>
> Replying to [comment:14 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?
> > > {{{#!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.
>
> 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?
> > > {{{#!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.
>
> 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,
--
Ticket URL: <http://bind10.isc.org/ticket/2155#comment:16>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list