BIND 10 #2157: Create an interface to pass statistics counters in Auth module
BIND 10 Development
do-not-reply at isc.org
Tue Jan 29 11:48:38 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:47 jinmei]:
> Replying to [comment:46 y-aharen]:
>
> I made a few minor cleanups. Please check them.
They looks good for me. Thank you.
> 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]);
> > > }
> > > }}}
I updated brief notes; it indicates explicitly what is the attribute
(git 9cbc650).
> > > - `setRequestSig`: if it's specific to TSIG, I'd rename it
> > > `setRequestTSIG` or setRequestTSIGAttributes` or something.
I renamed it to setRequestTSIG (git 825e49f) and
setResponseSigTSIG to setResponseTSIG (git a2cb2c8).
> > > - `setResponseSigTSIG`: do we need the `is_tsig_signed` parameter?
Yes, it is intuitive to pass a boolean parameter whether the response is
signed with TSIG.
> > > '''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 = ''
> }}}
I added a note to explain why it is needed (ab9c7fa).
> > > - 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.
I applied the patch (git c0fa34d). Thank you.
I also made minor changes to notes; wording (there're tree nodes and
branch nodes) and typo fix (calll -> call).
> > > Unrelated to this branch:
> > > - counter_dict.h should have more documentation.
I updated doxygen comments (git 819407f).
> > > - (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;
> > > }
> > > }}}
I understood the above as we should *not* use anonymous namespace in a
header file. I updated the branch (git 3476033).
> 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.
> }}}
I updated doxygen comment (git ce5cf26).
> 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?
Please see above.
> '''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.
I updated the branch to use boost::optional and only increment the
appropriate counter only if opcode is set (git 9aaf0f9).
> - 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.
> '''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.
> }}}
I removed the one in .cc (git dbf019c).
> '''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.
Because they're assured in the test surface with
checkCountersAreInitialized().
And yes, checkAllRcodeCountersZero() is unused; I removed it (git
daccf2d).
> - in the "response" test, it's not obvious why the expected counter
> values are 3. Please comment.
I added a note to describe it (git ed014f2).
> - 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).
> - Do we need to explicitly set this to 0?
> {{{#!cpp
> // XXX: with the current implementation, EDNS0 is omitted in
> // makeErrorMessage.
> expect["response.edns0"] = 0;
> }}}
It's not necessary, but I thought it's better to explicitly show what
counter
item is expected.
> '''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).
> '''(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)
> }}}
No, it's not. It didn't work if only 'counter' parameter is given.
I fixed it (git f91f956).
> - 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.
(Apart from I google Python methods very frequently) I added a comment
(git f1b061b).
Thanks,
--
Ticket URL: <http://bind10.isc.org/ticket/2157#comment:49>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list