BIND 10 #2157: Create an interface to pass statistics counters in Auth module
BIND 10 Development
do-not-reply at isc.org
Thu Jan 31 13:34:40 UTC 2013
#2157: Create an interface to pass statistics counters in Auth module
-------------------------------------+-------------------------------------
Reporter: y-aharen | Owner:
Type: enhancement | jinmei
Priority: medium | Status:
Component: b10-auth | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130205
Sub-Project: DNS | Resolution:
Estimated Difficulty: 8 | CVSS Scoring:
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 => jinmei
Comment:
Hello,
Replying to [comment:50 jinmei]:
> Replying to [comment:49 y-aharen]:
>
> > > > > - "EDNS attributes" sounds too broad for what this method does.
> > > > > method name sounds misleading too (as if it returns an EDNS
object
> > > > > or something). Same for setRequestEDNS0 and `RequestSigTSIG`.
> > > > > and `ResponseSigTSIG`.
> > > > > {{{#!cpp
> > > > > /// \brief Return EDNS attribute of the request.
> > > > > /// \return true if EDNS version of the request is 0
> > > > > /// \throw None
> > > > > bool getRequestEDNS0() const {
> > > > > return (bit_attributes_[REQ_IS_EDNS_0]);
> > > > > }
> > > > > }}}
> > I updated brief notes; it indicates explicitly what is the attribute
> > (git 9cbc650).
>
> What about the method name? It still has the same misleading name.
> Naming may be ultimately a matter of taste, but I'd name it, e.g.
> `requestHasEDNS0`. Same for other `getXXX` methods.
I'd prefer getXXX as requestHasXXX is confusing; I can't distinguish
whether it's a getter or setter. I propose getRequestHasEDNS0 and similar.
> > > '''statistics.cc'''
>
> > > - Likewise, the try block in `incResponse()` doesn't seem to be
> > > correct. (as far as I understand it) `incResponse()` is called
only
> > > when a response is created (and even sent), which should mean a
> > > valid opcode should have been recorded; if not that should mean
> > > there's an internal bug and should rather result in an exception.
> > I think it's not true in the following situation:
> > If a short message is sent and there is no opcode field, FormErr will
> > be returned; valid opcode is not recorded but a response is created.
>
> I don't understand the scenario. Do you mean if the received
> message(-like thing) is too small to even contain the basic DNS header
> (where opcode is stored)? In that case we simply drop it, not
> returning anything:
>
> {{{#!cpp
> try {
> message.parseHeader(request_buffer);
> [...]
> } catch (const Exception& ex) {
> LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_HEADER_PARSE_FAIL)
> .arg(ex.what());
> impl_->resumeServer(server, message, stats_attrs, false);
> return;
> }
> }}}
Updated the branch with ednsBadVers case below.
> > > '''auth_srv_unittest.cc'''
>
> > > - ednsBadVers: this looks odd
> > > {{{#!cpp
> > > expect["opcode.other"] = 1;
> > > }}}
> > > As far as I understand it,it should be opcode of query.
> > Yes it was wrong; it shouldn't increment opcode counter.
> > If I understood it correctly, If EDNS version is unsupported, any
> > DNS parameters cannot be interpreted; rcode is expanded with EDNS0, so
> > opcode could be as well in a future version of EDNS. I added a note
for it
> > in the manual (git 5d31621).
>
> Where is it documented? Any magic could happen with EDNS version 1 or
> higher, so some special behavior for such exceptional cases may be
> justified in general, but I don't want to rely on mere guesswork.
I understood as it should collect opcode even if the packet is not
understandable. Updated the branch to fetch opcode after header part
check,
before parsing message body and TSIG check (git dc6a6a9).
> Comments on the revised branch follow. First, I made a few editorial
> changes.
They looks OK for me, thank you.
> '''statistics.h'''
>
> - what's the purpose of the return type of getRequestOpCode?
> {{{#!cpp
> const boost::optional<const isc::dns::Opcode&> getRequestOpCode()
const {
> }}}
> I don't see the point of it, and it looks like involving some non
> trivial conversion from `optional<Opcode>` to
> `options<const Opcode&>`. Also, we shouldn't need the if-else
> cases; we can simply return `req_opcode_`. Further, it now
> shouldn't throw anything. Combining these, I'd revise it to:
> {{{#!cpp
> /// \brief Return opcode of the request.
> ///
> /// \return opcode of the request wrapped with boost::optional; it's
> /// converted to false if Opcode hasn't been set.
> /// \throw None
> const boost::optional<isc::dns::Opcode>& getRequestOpCode() const {
> return (req_opcode_);
> }
> }}}
I understood the point and updated the branch (git 59d2798).
> - If we use address family the doc should be updated:
> {{{#!cpp
> /// \brief Get IP version carrying a request.
> /// \return IP version carrying a request (AF_INET or AF_INET6)
> /// \throw None
> int getRequestIPVersion() const {
> return (req_ip_version_);
> }
> }}}
> same for set.
I updated the branch (git 7d28f0a).
> - bogus (or unexpected) values aren't checked:
> {{{#!cpp
> void setRequestIPVersion(const int ip_version) {
> req_ip_version_ = ip_version;
> }
> }}}
> same for the transport protocol.
I added checks for them (git fb2a1a9).
> - setRequestTSIG: (didn't notice before) the restriction isn't
> documented:
> {{{#!cpp
> void setRequestTSIG(const bool signed_tsig, const bool badsig) {
> assert(!(!signed_tsig && badsig));
> }}}
> Also, assert() seems to be too strong compared to other usage of
> assert() of ours. (We generally use assert() only for things like
> in-class integrity check failures, not invalid param-like errors)
I understood the point and updated the branch (git bfad7cb).
> '''statistics_unittest.cc.pre'''
>
> - This seems to have to be updated
> {{{#!cpp
> // Test these patterns:
> // ipversion protocol
> // -----------------
> // ipv4 udp
> // ipv6 udp
> // ipv4 tcp
> // ipv6 tcp
> }}}
I updated it (git 20255f3).
> '''counter.h'''
>
> - If you pass `InitialValue` to the vector constructor you cannot use
> this trick:
> {{{#!cpp
> static const unsigned int InitialValue = 0;
> }}}
> because it requires a reference to this variable. g++ is seemingly
> lenient but that's actually prohibited usage, and clang++ rejects
> it. Also, since `InitialValue` is private I think it's not a good
> idea to mention it in the public doc anyway:
> {{{#!cpp
> /// elements. The counters will be initialized with \a InitialValue;
> }}}
> (Although I generally don't like magic numbers) I guess in such a
> simple case we can simply use a constant numeric of 0:
> {{{#!cpp
> counters_(items, 0)
> }}}
> (and remove `InitialValue`)
OK, I updated the branch (git 810a54a).
> '''counter_dict.h'''
>
> - `InitialValue` isn't defined
> {{{#!cpp
> /// Each counter has \a items elements. The counters will be
initialized
> /// with \a InitialValue; which is defined as 0.
> }}}
fixed in 810a54a.
> - what's the purpose of `elements_`? It doesn't seem to be used:
> {{{#!cpp
> std::vector<std::string> elements_;
> }}}
It's unused, removed it (git 1954d8d).
> - addElement: whay using count()?
> {{{#!cpp
> if (dictionary_.count(name) != 0) {
> }}}
> Due to the property of std::map, the complexity is the same, but
> in this context I believe checking find() != end() is a more common
> idiom.
OK, I updated the branch (git 722fbab).
> - is this true? how could an exception be thrown? from
> iterator_facade?
> {{{#!cpp
> /// This constructor is mostly exception free. But it may
still
> /// throw a standard exception if memory allocation fails
> /// inside the method.
> ConstIterator() {}
> }}}
I couldn't find it throws from documentation. I removed the note
(git 2755265).
Thanks,
--
Ticket URL: <http://bind10.isc.org/ticket/2157#comment:52>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list