BIND 10 #2429: support $TTL and TTL guessing in dns::MasterLoader

[BIND 10 Development]

#2429: support $TTL and TTL guessing in dns::MasterLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Here are my comments:

 * Can `RRTTL::createFromText()` return the error string in an argument
 too? Otherwise how does the caller find why it failed?

 * In the internal `parseTTLStr()` (better renamed to the complete
 'ParseTTLString'), we could expect `error_txt` to always be non-NULL and
 remove the multiple cases of checks like:
 {{{
 +        if (error_txt != NULL) {
 +            *error_txt = "Empty TTL string";
 +        }
 }}}

 In the existing NULL case, this could result in string assignments when no
 error is returned, so maybe this is not a good idea. I just want to point
 it out.

 * As `RRTTL` encapsulates a simple ttl value as data, I recommend changing
 from the static `RRTTL::createFromText()` to a method that sets its
 object's value from the passed string. This would lead to cleaner code I
 think. (It's also best to avoid making interfaces that may return a new
 allocation, i.e., will sometimes and won't sometimes.) Something like:

 {{{#!c++
 class RRTTL {
 // ...
 bool setFromText(const string& ttlstr) {
     return (parseTTLStr(ttlstr, ttlval_, NULL));
 }
 // ...
 };

 }}}

 In the following code:

 {{{
 +    if (parseTTLStr(ttlstr, ttlval, NULL)) {
 +        if (placeholder != NULL) {
 +            *placeholder = RRTTL(ttlval);
 +            return (placeholder);
 +        }
 +        return (new RRTTL(ttlval));
      }
 }}}

 an `RRTTL` seems to be constructed again, assigned to what placeholder
 points (though it may be optimized to be constructed in-place.. I'm not
 sure). Anyway, there are 2 calls to `parseTTLStr()`.

 * In `RRTTLTest.createFromText`, assuming you keep the existing code:
 {{{
 +    // If placeholder is non NULL, it will be overwritten
 +    RRTTL ttl(3600);
 +    EXPECT_NE(static_cast<RRTTL*>(NULL), RRTTL::createFromText("1800",
 &ttl));
 +    EXPECT_EQ(RRTTL(1800), ttl);
 }}}

 Instead of the non-NULL check, it may be better to check that it
 specifically returns `&ttl` (placeholder arg) as that's the expected
 behavior.

 * In `MasterLoader`, `MAX_TTL` is better moved to `RRTTL` class and
 becomes a static const object there (like `LabelSequence::WILDCARD` for
 example).

 * This comment is not accurate; I guess it's a mistake in conveying the
 meaning, and not an intended statement:
 {{{
 +    // zone file ($TTL, the RR's TTL field, or the SOA minimum).  RFC2181
 +    // Section 8 limits the range of TTL values to unsigned 32-bit
 integers,
 +    // and prohibits transmitting a TTL field exceeding this range.  We
 }}}

 The mentioned RFC limits it to 2^31-1, which is an unsigned 31-bit
 integer, though it is stored in a quad. Perhaps it could be written to
 just say "... limits the range of TTL values to 0x7ffffffff".

 Also, resetting it to 0 when MSB is set is mentioned by the RFC, so I
 don't know if this comment is accurate too:
 {{{
 +    // parsing/loading, following what BIND 9 does.  Resetting it to 0
 +    // at this point may not be exactly what the RFC states, but the end
 +    // result would be the same.  Again, we follow the BIND 9's behavior
 here.
 }}}

 * How does the last item apply to `SOA.getMinimum()`? In case the MSB of
 the 32-bit representation is set, is it supposed to return 0?

 * Instead of doing the above limit checks in the master loader, wouldn't
 it be more correct to handle it inside `RRTTL` itself (during
 construction/assignment) and adapt code accordingly, esp. as it already
 clamps at [0, 0xffffffff]?

 * This comment does not seem to indicate what the method is doing:
 {{{
 +    // Set/reset the default TTL.  Either from $TTL or SOA minimum TTL.
 +    // see LimitTTL() for parameter post_parsing.
 +    void setDefaultTTL(const RRTTL& ttl, bool post_parsing) {
 }}}

 $TTL and SOA minimum TTL are unknown within the method. The comment should
 perhaps be expanded to say the caller is expected to pass these (in the
 ttl argument). Same for `setCurrentTTL()` too.

 * `parseTTLStr()` clamps TTLs to [0, 0xffffffff]. `setCurrentTTL(const
 RRTTL&)` does not seem to call `limitTTL()`. I guess this is not a problem
 as the `default_ttl_` is passed to this method always, but it's perhaps
 best to add a comment.

 * The warning message "using RFC1035 TTL semantics" should be elaborated
 to describe what case is being handled. A user seeing this warning will
 wonder about it otherwise.

 * I've pushed a minor typo fix.

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