BIND 10 #1775: update in-memory getAdditional() to handle wildcard match for additional names

[BIND 10 Development]

#1775: update in-memory getAdditional() to handle wildcard match for additional
names
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  high   |  Sprint-20120403
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 > I have few notes. First, the cppcheck fails here:
 > {{{#!c++
 > src/lib/datasrc/memory_datasrc.cc:376: check_fail: Possible null pointer
 dereference: node (error,nullPointer)
 > }}}
 >
 > I believe this probably can't happen to be NULL, but we might want to
 assert or check and throw in case it is.

 Hmm, my version of cppcheck (1.52) didn't complain about that, but I
 added an assert().  We have similar assert, so this would actually be
 more consistent with other parts of the code.

 > Also, it is not obvious why the size of the tree that is created to hold
 the wildcard-constructed additionals can't grow and grow (I thing I found
 a reason after some time of thinking, though). Would it be possible to
 comment on why the size is bounded by the characteristics of the zone?

 I'm not sure if I understand the concern, but I tried to add some more
 explanations at d3e876a.  Does that address it?

 > And, I do not think this is safe:
 > {{{#!c++
 >         } else {
 >             // For the tradeoff between safety and performance (of the
 >             // const version), we discard the constness here.  In
 practice
 >             // this should be okay because internally this node was
 retrieved
 >             // from the zone tree as a mutable pointer anyway.
 >             *nodep = const_cast<DomainNode*>(result.node);
 >         }
 > }}}
 >
 > To be clear, I do not see any specific problem. But every time I see a
 const_cast, I get a bad feeling about it.

 Yeah I know, and I didn't like that either.  But when I first explored
 several ways (including template) to avoid the cast, what I came up
 with was either requesting code duplicate or additional code for the
 performance sensitive path (which actually slowed it down, although
 for a small amount) or quite unreadable code, so I gave up and adopted
 the cast.

 As explicitly commented, however, I re-read the code, and found one
 way that may not be so ugly or sacrifice the performance (40d727b).
 Would it be better?

 In any case, I'd note that one fundamental background issue is that
 RBTree::find() (which findNode() internally uses) breaks constness:
 even if it's a const member function, it actually returns a mutable
 pointer via 'node' which points to its internal data.  Ideally, we
 should provide const and non const versions of RBTree::find().  Then
 the issue around findNode() could be solved much more cleanly.  But
 that would also be non trivial (if we also want to avoid logic
 duplicate), and would be a separate issue anyway.

 If the latest branch addresses the comments, I think I've addressed
 all points so far.  But I plan to add some more things after that, so
 regardless of whether they are okay, I'll still continue to work on
 this ticket (but it'd be nice if you could confirm the updates so far).

 Thanks,

-- 
Ticket URL: <http://bind10.isc.org/ticket/1775#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development