BIND 10 #2429: support $TTL and TTL guessing in dns::MasterLoader
BIND 10 Development
do-not-reply at isc.org
Tue Dec 18 04:15:20 UTC 2012
#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
-------------------------------------+-------------------------------------
Comment (by jinmei):
Thanks for the review.
Replying to [comment:7 muks]:
I'll first concentrate on things independent from `createFromText().`
> * In `MasterLoader`, `MAX_TTL` is better moved to `RRTTL` class and
becomes a static const object there (like `LabelSequence::WILDCARD` for
example).
Okay, done.
> * 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".
Good point, fixed.
> 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.
> }}}
I tried to clarify the intent in the updated comment.
> * 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?
There could be several possibilities, but for now I'd simply keep
using the actual value. At least within the master loader we reset it
to 0 if we need to use it as the RR TTL, so it shouldn't cause
violation.
> * 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]?
Yeah, I considered that path, and maybe that makes more sense. But
for this task I'd keep the current interface. For one thing, this
behavior is compatible with BIND 9. Also, this change could have much
wider effect on other parts of BIND 10. I don't like to trigger a
huge number of regressions due to that. It's also not clear how we
should behave if we see such values from wire; e.g., whether we should
throw or internally reset to 0 at the time of construction.
I propose just creating a separate ticket on these, including the
design discussion.
> * 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.
For setDefaultTTL(), I tried to clarify it in a revised comment.
For setCurrentTTL, I guess you mean the version setting it to the
default TTL. On looking into it further, I thought it makes more
sense to redefine its responsibility to actually resetting it to the
default, and to rename the method accordingly. I made that change
until I ended up removing it at 8b616d0 (see the `createFromText`
discussion below).
> * `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.
I don't understand the first sentence, but anyway, I believe the
revised version of `setCurrentTTL` addressed the rest of this bullet.
> * 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.
It was derived from the BIND 9's log message. But in case that didn't
look clear enough, I tried to be a bit more verbose.
> * I've pushed a minor typo fix.
This looks good, thanks.
Now, the issues related to `createFromText()`. In the end, I've
totally revised the interface at commit 23cb251. If the revised
version can be approved quickly, that would be nice, and I'll merge
everything. But if it requires further discussions (which is
perfectly fine), I propose deferring everything related to extension
(including the previous attempt) and merge the rest of the branch to
make loadzone-ng workable.
First, the most substantial one:
> * 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));
> }
> // ...
> };
>
> }}}
While I didn't really love the original interface, I don't like this
interface either. In general, I'd like to avoid this type of setter
method, breaking the class integrity in some non trivial way. All
`RRxxx` classes were designed to not allow such disruption (they don't
have the default constructor for a similar reason).
Also, in this approach we first need to have a dummy `RRTTL` object
(like `RRTTL(0)`) which is a valid object but can't be used until we
call `setFromText`. In general, I'd avoid such a magic placeholder
object; it can be a very easy cause of bugs where we accidentally leak
the object in an externally visible context.
To this end, where you also considered the original interface ugly, my
suggestion is to use boost::optional. This is a tool for this exact
kind of purpose. I didn't choose it in the first attempt because I
wanted to minimize further dependency on boost stuff in our public
interface. But now that we seem to agree the original interface
wasn't acceptably clean, I'd rather propose relying on the higher
level abstraction at the cost of increasing dependency.
> * 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.
These two are indeed related, and it was actually intentional not to
return error string and to have less cleaner `if` blocks. I tried to
explain it in the doxygen doc. Please refer to it.
I also renamed `parseTTLStr` to `parseTTLString` as suggested.
> 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()`.
I'm not sure if I fully understand this, but at this line:
{{{#!cpp
*placeholder = RRTTL(ttlval);
}}}
what happens is a construction of `RRTTL` and a call to the assignment
operator on `*placeholder`. Since the assignment operator is a
trivial one and `RRTTL` only has a uint32_t member, I believe the
overhead of the assignment is marginal. I also believe the
construction of `RRTTL` from an integer is marginal.
I don't understand the "2 calls" part of the comment.
And, in any case, I suspect these points are moot in the revised
version.
> * 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.
This point has become moot in the revised version.
--
Ticket URL: <http://bind10.isc.org/ticket/2429#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list