BIND 10 #2157: Create an interface to pass statistics counters in Auth module
BIND 10 Development
do-not-reply at isc.org
Wed Jan 16 04:02:31 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-20130122
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:46 y-aharen]:
I made a few minor cleanups. Please check them.
These don't seem to be addressed:
> > - "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]);
> > }
> > }}}
> > - `setRequestSig`: if it's specific to TSIG, I'd rename it
> > `setRequestTSIG` or setRequestTSIGAttributes` or something.
> > - `setResponseSigTSIG`: do we need the `is_tsig_signed` parameter?
> > '''gen-statisticsitems.py'''
> > - convert_list in generate_docfile: I don't understand the purpose of
> > these changes:
> > {{{#!diff
> > - command.tail = ' '
> > + command.tail = ''
> > ...
> > + sim_para.text = sim_para.text.rstrip()
> > + if not prev is None:
> > + prev.tail = prev.tail.rstrip()
> > }}}
> > please leave comments. Also, in general, making a single big commit
> > like this one just saying "address review comment" is not good.
> I added notes (git acc630c).
This change doesn't seem to address my point. The added comment is
mostly a straightforward translation from Python to English about what
it does. For example, this doesn't explain why we need to bother to do
this, and, in the context of the above change, what was wrong with the
previous code:
{{{#!python
# append empty string for trailing text
command.tail = ''
}}}
>
> > - generate_cxx: (discussion)
> >
> > >> generate_cxx: if I understand it correctly, its convert_list()
doesn't work correctly only up to 1 recursion.
> >
> > > I think it should work. Anyway, if it doesn't work, unittest,
systest and/or lettuce fails.
[...]
> > So, if we know it really works with any number of recursion, that's
> > fine; otherwise, we should either fix it or explicitly fail on the
> > unexpected cases. My current guess is it doesn't work, and in which
> > case I'd just make the script fail explicitly.
> If there are items in 2 or more depth, the recursion at the point
happens
> twice.
Yeah after closely looking at with some experiments I finally
understood it works. But, at the risk of my incompetence of code
reading, it's very difficult to understand. convert_list handles
three mutable variables (msg_counter_types, item_names_current,
item_names), updates them different ways (sometimes replacing it with
a return value of recursive calls, sometimes pass a single list object
and let recursive calls update it, using two confusing variables,
item_names_current and item_names_current_ (this is one reason for me
for misunderstanding it), and redundant code logic after convert_list
(finalizing item_names outside the function, even if the function also
does the same things for others). And there's literally *no comment*.
After understanding it, I'd like to propose a revised version as shown
in the attached patch. The overall amount of code is not different,
and the sense of readability may be a matter of subjective feeling,
but I personally see it much more understandable: it addressed all of
the above points, at least.
Are these addressed?
> > Unrelated to this branch:
> > - counter_dict.h should have more documentation.
> > - (partly because of the lack of doc) I'm not sure about the intended
> > usage of this file and `CounterDictionary`, but in general, we
> > should use an unnamed namespace in a header file:
> > {{{#!cpp
> > namespace {
> > typedef boost::shared_ptr<isc::statistics::Counter> CounterPtr;
> > typedef std::map<std::string, CounterPtr> DictionaryMap;
> > }
> > }}}
Some other comments follow:
'''statistics.h'''
- Counters::inc() Shouldn't we now remove this?
{{{#!cpp
/// This method is mostly exception free. But it may still throw a
/// standard exception if memory allocation fails inside the method.
}}}
Doesn't seem to be addressed.
"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.
/// \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]);
}
setRequestSig: if it's specific to TSIG, I'd rename it setRequestTSIG
or setRequestTSIGAttributes` or something.
setResponseSigTSIG: do we need the is_tsig_signed parameter?
'''statistics.cc'''
- This code doesn't seem to be correct:
{{{#!cpp
try {
server_msg_counter_.inc(
opcode_to_msgcounter[msgattrs.getRequestOpCode().getCode()]);
} catch (const isc::InvalidOperation& ex) {
// The exception will be thrown if OpCode is not set
// (e.g. message is short).
// Increment MSG_OPCODE_OTHER.
server_msg_counter_.inc(MSG_OPCODE_OTHER);
}
}}}
that is, it's cheating to count "opcode other" because there should
actually be no opcode in this case. I guess what we should do is to
test whether opcode is set or not (through the boost optional
interface) and increment the appropriate counter if and only if it's
set.
- 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.
'''tests/statistics_util.h,cc'''
- this comment is duplicated in .h and .cc:
{{{#!cpp
// Test if the counters has expected values specified in expect and the
others
// are zero.
}}}
'''auth_srv_unittest.cc'''
- in the "response" test, etc, why this change?
{{{#!diff
- checkAllRcodeCountersZero();
}}}
if we need or want to remove the calls, btw, we can even remove its
definition; checkAllRcodeCountersZero() isn't used at all in this
version of branch.
- in the "response" test, it's not obvious why the expected counter
values are 3. Please comment.
- ednsBadVers: this looks odd
{{{#!cpp
expect["opcode.other"] = 1;
}}}
As far as I understand it,it should be opcode of query.
- Do we need to explicitly set this to 0?
{{{#!cpp
// XXX: with the current implementation, EDNS0 is omitted in
// makeErrorMessage.
expect["response.edns0"] = 0;
}}}
'''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.
'''(lettuce) bind10_ctl.py'''
- is it intentional that `else` is specified?
{{{#!python
for level in depth:
output = find_value(output, level)
else:
found = find_value(output, counter)
}}}
- I suggest adding some comments for the notation of `step.hashes`:
{{{#!cpp
for item in step.hashes:
}}}
(what's expected there, etc). I didn't know it and had to google it
to understand the code. I suspect it's not only me.
--
Ticket URL: <http://bind10.isc.org/ticket/2157#comment:47>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list