BIND 10 #2384: extend RRTTL "from text" constructor so it can accpet 1H, 2D, etc.

[BIND 10 Development]

#2384: extend RRTTL "from text" constructor so it can accpet 1H, 2D, etc.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121120
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  loadzone-ng                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 vorner]:

 > > I think it's worth a separate entry as it's an extension to a public
 > > API (RRTTL class itself is public), but I'd leave it to you.
 >
 > OK, then. What about:
 >
 > {{{
 > [func]                vorner
 > TTLs can be specified with units as well as number of seconds now. This
 allows
 > specifications like "1D3H".
 > }}}

 Works for me.

 > >   For example, this implementation accepts "-1S1H".  Is it okay?
 > >   How about this? "9223372036854775807S9223372036854775807S2S"
 > >   (which would result in '0').  FYI: as far as I can see BIND 9 would
 > >   reject it, although even BIND 9 doesn't seem to completely reject
 > >   all overflow cases.
 >
 > It could be OK (because these are specially crafted cases abusing the
 knowledge
 > of internals, and it is unlikely anyone would hit them by accident), but
 I
 > added some more checking anyway.

 I have several comments and questions on the additional check:

 {{{#!cpp
             // The partial value is still in range (the value can only
 grow,
             // so if we get out now, it won't get better).
             //
             // Any valid uint32_t number must have at most 10 digits. If
 it
             // has more, it could wrap around the int64_t silently (at
 least
             // in theory, some compilers seem to throw from lexical_cast).
             if (unit - pos > 10 || value < 0 || val < 0 ||
                 val > 0xffffffff) {
                 isc_throw(InvalidRRTTL, "Part of TTL out of range: " <<
                           ttlstr);
             }
 }}}

 - I didn't understand the intent of the first paragraph of the
   comment.  You mean "Check if the partial value..."?   I didn't
   understand what this means either: "if we get out now, it won't get
   better"
 - is the 10-digit check necessary?  at least the current set of tests
   don't require this check in my environment (is that related to the
   comment about lexical_cast?).  Isn't it enough if `value`,
   `multiply * value` and `val` are all uint32_t numbers?
 - the previous point makes me aware of another minor corner case: what
   if the number has redundant preceding zero's?  e.g. "003600"?  FYI,
   BIND 9 accepts it.  note that I'm not necessarily requesting we
   accept it too.  I think it's okay to say, e.g. that's an undefined
   behavior.  But developers need to know the possibility.
 - Also, is `unit - pos` safe?  `operator-` only works for random
   access iterators, and (in my understanding) we cannot assume the
   type of string iterators.

 '''About performance related comments'''

 I'm basically okay with skipping my points, but I still suggest
 considering a special case of digit-only TTLs.  From my quick
 benchmark if we simply do something like this:
 {{{#!cpp
             const string::const_iterator unit = find_if(pos, end,
 myIsalpha);
             if (unit == end && pos == beg) { // beg = ttlstr.begin()
                 value = boost::lexical_cast<int64_t>(ttlstr);
                 break;
             }
 }}}
 it's 40% faster than the current version for the input of "3600".
 I'm attaching the benchmark code in case you're interested.

 dnssec-signzone produces digit-only TTLs, and produces TTLs for all
 RRs, so, while I understand the risk of premature optimization, I
 think it's worth doing.

 '''Comments on the revised branch'''

 - I made one minor editorial cleanup.
 - This comment now seems to be obsolete:
 {{{#!cpp
             // Now extract the number, defaut to 1 if there's no digit
 }}}
 - This case is not allowed in BIND 9:
 {{{#!cpp
     /// The
     /// unit at the last number can be omitted in case it is seconds.
 }}}
   and in any case it doesn't seem to be tested.
 - In test, this test case is duplicated:
 {{{#!cpp
     EXPECT_EQ(4294967295, RRTTL("4294967295").getValue());
 }}}
 - Although probably obvious, I'd explain what "WDHMS" mean in the doc.

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