BIND 10 #2157: Create an interface to pass statistics counters in Auth module
BIND 10 Development
do-not-reply at isc.org
Wed Jan 30 04:17:56 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
-------------------------------------+-------------------------------------
Comment (by 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.
> > '''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;
}
}}}
> > '''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.
> > '''auth_srv.cc'''
> >
> > >> Is it OK to revert the change above and pass address family and
transport
> > >> protocol directly to setRequestIPVersion and
setRequestTransportProtocol?
> >
> > I think so.
> I reverted the change (git d792272).
Ah...I was probably not clear enough. I meant it's okay to revert to
the "behavior" of passing the address family and protocol and letting
the statistics class deal with them, not just "git revert" that
change. By doing the latter we still have the issue of what to do
with bogus values. See also the comments below.
Comments on the revised branch follow. First, I made a few editorial
changes.
'''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_);
}
}}}
- 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.
- bogus (or unexpected) values aren't checked:
{{{#!cpp
void setRequestIPVersion(const int ip_version) {
req_ip_version_ = ip_version;
}
}}}
same for the transport protocol.
- 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)
'''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
}}}
'''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`)
'''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.
}}}
- what's the purpose of `elements_`? It doesn't seem to be used:
{{{#!cpp
std::vector<std::string> elements_;
}}}
- 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.
- 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() {}
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/2157#comment:50>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list