BIND 10 #2310: In-memory version of ZoneFinder::findAtOrigin()

[BIND 10 Development]

#2310: In-memory version of ZoneFinder::findAtOrigin()
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:11 vorner]:

 > Could we use the `htonl` instead of the in-house implementation of
 `setTTLInNetOrder`? It just duplicates call (and the library one could be
 inlined as compiler built-in). Note that the htonl explicitly uses
 `uint32_t`.

 I don't like to introduce such in-house things, either, but when we
 previously discussed it there was concern about portability of
 htonl/htonl (and their variants).  That's actually why we use in-hour
 code for buffer.h.  They are probably sufficiently portable today
 (even Windows seems to have it), but officially not in C++ stdlib, for
 example.

 One alternative is to borrow wrapper functions of ASIO, but including
 ASIO header files has its own risks.

 So, I'm personally okay with using the library functions, but that's
 not about this specific case, and we should unify the policy and be
 consistent throughout our code.  For now I didn't touch it.

 > Would it make sense to cache the result of createTTLFromData? How heavy
 is the creation of the TTL object?

 Do you mean storing TTL in the form of RRTTL in `ZoneData`?  If so,
 for in-memory it doesn't make much sense anyway because we mainly use
 it only for wire-format rendering (`getTTL()` (and `toText()` which
 uses the getTTL) isn't expected to be called except for rare cases).

 > The `findAtOrigin` method is a lot simpler than the generic `find`. It
 has fewer cases. After thinking about it, I got to conclusion the other
 cases can not happen, because they generally handle things that can't
 legally exist in the origin (like not having the node or having CNAME
 there). But I think this reasoning should be described in comment in the
 method, otherwise another reader will come and stop to think if the other
 cases are just missing.

 Ah, yes, I intended to add some level comments but apparently I forgot
 it while working on other parts of the branch.  Now I added some more
 comments.

 > Is this function correct? Can't SOA be added to other places than just
 origin? Or, if it doesn't matter, is it OK to just pass `ROOT_NAME` every
 time, even to SOA?
 [...]
 > This is similar. Could we just simplify it and pass the origin always?

 In the former case we can actually simplify, and in the latter case we
 need to separate the cases.  I made some refactoring and additional
 comments to simplify and clarify the points.

 > Should there be a test where `min_ttl` is requested and the TTL of the
 RRset is lower, so it wins?

 Good idea, added.

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