BIND 10 trac2148, updated. d84e5114e84992b169fec6fae8b8d7a9f0a29399 [2148] Address review comments
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Jul 27 14:18:51 UTC 2012
The branch, trac2148 has been updated
via d84e5114e84992b169fec6fae8b8d7a9f0a29399 (commit)
from f01bc8a1bf5c5ed85a0695d47e64854d4bf3d1b6 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit d84e5114e84992b169fec6fae8b8d7a9f0a29399
Author: Jelte Jansen <jelte at isc.org>
Date: Fri Jul 27 16:18:26 2012 +0200
[2148] Address review comments
- typos
- few code fixes
- test updates
-----------------------------------------------------------------------
Summary of changes:
src/lib/dns/labelsequence.cc | 18 +----
src/lib/dns/labelsequence.h | 7 +-
src/lib/dns/tests/labelsequence_unittest.cc | 99 +++++++++++++++++++++++----
3 files changed, 93 insertions(+), 31 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dns/labelsequence.cc b/src/lib/dns/labelsequence.cc
index 13cbaf3..7cc5ad6 100644
--- a/src/lib/dns/labelsequence.cc
+++ b/src/lib/dns/labelsequence.cc
@@ -342,18 +342,17 @@ LabelSequence::extend(const LabelSequence& labels,
uint8_t buf[MAX_SERIALIZED_LENGTH])
{
// collect data to perform steps before anything is changed
- bool absolute = isAbsolute();
size_t label_count = last_label_ + 1;
// Since we may have been stripped, do not use getDataLength(), but
// calculate actual data size this labelsequence currently uses
size_t data_pos = offsets_[last_label_] + data_[offsets_[last_label_]] + 1;
// If this labelsequence is absolute, virtually strip the root label.
- if (absolute) {
+ if (isAbsolute()) {
data_pos--;
label_count--;
}
- size_t append_label_count = labels.getLabelCount();
+ const size_t append_label_count = labels.getLabelCount();
size_t data_len;
const uint8_t *data = labels.getData(&data_len);
@@ -372,18 +371,7 @@ LabelSequence::extend(const LabelSequence& labels,
}
// All seems to be reasonably ok, let's proceed.
-
- // Note: In theory this could be done with a straightforward memcpy.
- // However, one can extend a labelsequence with itself, in which
- // case we'd be copying overlapping data (overwriting the current last
- // label if this LabelSequence is absolute). Therefore we do this
- // manually, and more importantly, backwards.
- // (note2: obviously this destroys data_len, don't use below,
- // or reset it)
- while (--data_len) {
- buf[data_pos + data_len] = data[data_len];
- }
- buf[data_pos + data_len] = data[data_len];
+ memmove(&buf[data_pos], data, data_len);
for (size_t i = 0; i < append_label_count; ++i) {
buf[Name::MAX_WIRE + label_count + i] =
diff --git a/src/lib/dns/labelsequence.h b/src/lib/dns/labelsequence.h
index 1603753..186bda6 100644
--- a/src/lib/dns/labelsequence.h
+++ b/src/lib/dns/labelsequence.h
@@ -268,12 +268,13 @@ public:
/// \brief Extend this LabelSequence with the given labelsequence
///
- /// The given labels are added to the name data, and internal offset
- /// data is updated accordingly.
+ /// The given labels are appended to the name data, and internal
+ /// offset data is updated accordingly.
///
/// The data from the given LabelSequence is copied into the buffer
/// associated with this LabelSequence; the appended LabelSequence
- /// can be released if it is not needed for other operations anymore.
+ /// (the 'labels' argument) can be released if it is not needed for
+ /// other operations anymore.
///
/// If this LabelSequence is absolute, its root label will be stripped
/// before the given LabelSequence is appended; after extend(),
diff --git a/src/lib/dns/tests/labelsequence_unittest.cc b/src/lib/dns/tests/labelsequence_unittest.cc
index 599f673..8f7329b 100644
--- a/src/lib/dns/tests/labelsequence_unittest.cc
+++ b/src/lib/dns/tests/labelsequence_unittest.cc
@@ -38,21 +38,23 @@ namespace {
void check_equal(const LabelSequence& ls1, const LabelSequence& ls2) {
NameComparisonResult result = ls1.compare(ls2);
EXPECT_EQ(isc::dns::NameComparisonResult::EQUAL,
- result.getRelation()) << ls1.toText() << " <> " << ls2.toText();
- EXPECT_EQ(0, result.getOrder()) << ls1.toText() << " <> " << ls2.toText();
+ result.getRelation()) << ls1.toText() << " != " << ls2.toText();
+ EXPECT_EQ(0, result.getOrder()) << ls1.toText() << " != " << ls2.toText();
EXPECT_EQ(ls1.getLabelCount(), result.getCommonLabels());
}
// Common check for general comparison of two labelsequences
void check_compare(const LabelSequence& ls1, const LabelSequence& ls2,
isc::dns::NameComparisonResult::NameRelation relation,
- size_t common_labels) {
+ size_t common_labels, bool check_order, int order=0) {
NameComparisonResult result = ls1.compare(ls2);
EXPECT_EQ(relation, result.getRelation());
EXPECT_EQ(common_labels, result.getCommonLabels());
+ if (check_order) {
+ EXPECT_EQ(order, result.getOrder());
+ }
}
-
class LabelSequenceTest : public ::testing::Test {
public:
LabelSequenceTest() : n1("example.org"), n2("example.com"),
@@ -827,13 +829,13 @@ void stripLeftCheck(LabelSequence ls1, LabelSequence ls2, LabelSequence ls3) {
ls1.stripLeft(1);
check_compare(ls1, ls2, isc::dns::NameComparisonResult::SUPERDOMAIN,
- ls1.getLabelCount());
+ ls1.getLabelCount(), true, -1);
check_equal(ls2, ls3);
ls2.stripLeft(1);
check_equal(ls1, ls2);
check_compare(ls2, ls3, isc::dns::NameComparisonResult::SUPERDOMAIN,
- ls1.getLabelCount());
+ ls1.getLabelCount(), true, -1);
ls3.stripLeft(1);
}
@@ -847,12 +849,14 @@ void stripRightCheck(LabelSequence ls1, LabelSequence ls2, LabelSequence ls3) {
check_equal(ls2, ls3);
ls1.stripRight(1);
- check_compare(ls1, ls2, isc::dns::NameComparisonResult::NONE, 0);
+ check_compare(ls1, ls2, isc::dns::NameComparisonResult::NONE, 0,
+ false);
check_equal(ls2, ls3);
ls2.stripRight(1);
check_equal(ls1, ls2);
- check_compare(ls2, ls3, isc::dns::NameComparisonResult::NONE, 0);
+ check_compare(ls2, ls3, isc::dns::NameComparisonResult::NONE, 0,
+ false);
ls3.stripRight(1);
}
@@ -895,15 +899,30 @@ TEST_F(ExtendableLabelSequenceTest, extendableLabelSequence) {
LabelSequence ls2(example_org);
LabelSequence els(ls1, buf);
+ // ls1 is absolte, so els should be too
+ EXPECT_TRUE(els.isAbsolute());
+ check_equal(ls1, els);
ASSERT_EQ(ls1.getDataLength(), els.getDataLength());
stripLeftCheck(ls1, els, ls2);
stripRightCheck(ls1, els, ls2);
+
+ // Creating an extendable labelsequence from a non-absolute
+ // label sequence should result in a non-absolute label sequence
+ ls1.stripRight(1);
+ els = LabelSequence(ls1, buf);
+ EXPECT_FALSE(els.isAbsolute());
+ check_equal(ls1, els);
+
+ // and extending with the root label should make it absolute again
+ els.extend(LabelSequence(Name(".")), buf);
+ EXPECT_TRUE(els.isAbsolute());
+ check_equal(ls2, els);
}
// Test that 'extendable' LabelSequences behave correctly when initialized
// with a stripped source LabelSequence
-TEST_F(ExtendableLabelSequenceTest, extendableLabelSequenceStrippedSource) {
+TEST_F(ExtendableLabelSequenceTest, extendableLabelSequenceLeftStrippedSource) {
LabelSequence ls1(foo_bar_example_org);
LabelSequence ls2(foo_bar_example_org);
@@ -944,7 +963,8 @@ TEST_F(ExtendableLabelSequenceTest, extend) {
LabelSequence els(ls2, buf);
- check_compare(ls1, els, isc::dns::NameComparisonResult::COMMONANCESTOR, 1);
+ check_compare(ls1, els, isc::dns::NameComparisonResult::COMMONANCESTOR, 1,
+ true, -4);
els.extend(ls3, buf);
EXPECT_TRUE(els.isAbsolute());
@@ -955,23 +975,38 @@ TEST_F(ExtendableLabelSequenceTest, extend) {
// strip, then extend again
els.stripRight(2); // (2, 1 for root label, 1 for last label)
els.extend(ls3, buf);
+ EXPECT_TRUE(els.isAbsolute());
check_equal(ls1, els);
// Extending again should make it different
els.extend(ls3, buf);
- check_compare(ls1, ls2, isc::dns::NameComparisonResult::COMMONANCESTOR, 1);
+ EXPECT_TRUE(els.isAbsolute());
+ check_compare(ls1, els, isc::dns::NameComparisonResult::COMMONANCESTOR, 2,
+ true, 4);
// Extending with a non-absolute name should make it non-absolute as well
ls3.stripRight(1);
els.extend(ls3, buf);
EXPECT_FALSE(els.isAbsolute());
+ Name check_name("foo.bar.bar.bar");
+ LabelSequence check_ls(check_name);
+ check_ls.stripRight(1);
+ check_equal(check_ls, els);
+
// And try extending when both are not absolute
els.stripRight(3);
ls1.stripRight(1);
EXPECT_FALSE(els.isAbsolute());
els.extend(ls3, buf);
+ EXPECT_FALSE(els.isAbsolute());
check_equal(ls1, els);
+
+ // Extending non-absolute with absolute should make it absolute again
+ EXPECT_FALSE(els.isAbsolute());
+ els.extend(LabelSequence(Name("absolute.")), buf);
+ EXPECT_TRUE(els.isAbsolute());
+ check_equal(LabelSequence(Name("foo.bar.absolute")), els);
}
TEST_F(ExtendableLabelSequenceTest, extendLeftStripped) {
@@ -983,6 +1018,7 @@ TEST_F(ExtendableLabelSequenceTest, extendLeftStripped) {
els.stripLeft(1);
els.extend(ls3, buf);
+ EXPECT_TRUE(els.isAbsolute());
check_equal(ls2, els);
}
@@ -994,6 +1030,7 @@ TEST_F(ExtendableLabelSequenceTest, extendWithItself) {
LabelSequence els(ls1, buf);
els.extend(els, buf);
+ EXPECT_TRUE(els.isAbsolute());
check_equal(ls2, els);
// Also try for non-absolute names
@@ -1001,12 +1038,14 @@ TEST_F(ExtendableLabelSequenceTest, extendWithItself) {
els = LabelSequence(ls1, buf);
els.stripRight(1);
els.extend(els, buf);
+ EXPECT_FALSE(els.isAbsolute());
check_equal(ls2, els);
- // Once more, now start out with non-absolue
+ // Once more, now start out with non-absolute labelsequence
ls1.stripRight(1);
els = LabelSequence(ls1, buf);
els.extend(els, buf);
+ EXPECT_FALSE(els.isAbsolute());
check_equal(ls2, els);
}
@@ -1018,6 +1057,7 @@ TEST_F(ExtendableLabelSequenceTest, extendWithRoot) {
LabelSequence els(LabelSequence(ls1, buf));
check_equal(ls1, els);
els.extend(LabelSequence(Name(".")), buf);
+ EXPECT_TRUE(els.isAbsolute());
check_equal(ls1, els);
// but not if the original was not absolute (it will be equal to
@@ -1026,9 +1066,11 @@ TEST_F(ExtendableLabelSequenceTest, extendWithRoot) {
LabelSequence ls2(example_org);
ls2.stripRight(1);
els = LabelSequence(ls2, buf);
+ EXPECT_FALSE(els.isAbsolute());
els.extend(LabelSequence(Name(".")), buf);
+ EXPECT_TRUE(els.isAbsolute());
check_equal(ls1, els);
- check_compare(ls2, els, isc::dns::NameComparisonResult::NONE, 0);
+ check_compare(ls2, els, isc::dns::NameComparisonResult::NONE, 0, true, 3);
}
// Check possible failure modes of extend()
@@ -1051,9 +1093,25 @@ TEST_F(ExtendableLabelSequenceTest, extendBadData) {
ASSERT_EQ(245, els.getDataLength());
// Extending once more with 10 bytes should still work
els.extend(LabelSequence(Name("123456789")), buf);
+ EXPECT_TRUE(els.isAbsolute());
+
+ // Extended label sequence should now look like
+ const Name full_name(
+ "123456789012345678901234567890123456789012345678901234567890."
+ "123456789012345678901234567890123456789012345678901234567890."
+ "123456789012345678901234567890123456789012345678901234567890."
+ "123456789012345678901234567890123456789012345678901234567890."
+ "123456789.");
+ const LabelSequence full_ls(full_name);
+ check_equal(full_ls, els);
+
// But now, even the shortest extension should fail
EXPECT_THROW(els.extend(LabelSequence(Name("1")), buf), isc::BadValue);
+ // Check it hasn't been changed
+ EXPECT_TRUE(els.isAbsolute());
+ check_equal(full_ls, els);
+
// Also check that extending past MAX_LABELS is not possible
Name shortname("1.");
LabelSequence short_ls(shortname);
@@ -1061,7 +1119,22 @@ TEST_F(ExtendableLabelSequenceTest, extendBadData) {
for (size_t i=0; i < 126; ++i) {
els.extend(short_ls, buf);
}
+
+ // Should now look like this
+ const Name full_name2(
+ "1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1."
+ "1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1."
+ "1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1."
+ "1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1.1."
+ "1.1.1.1.1.1.1.");
+ const LabelSequence full_ls2(full_name2);
+ EXPECT_TRUE(els.isAbsolute());
+ check_equal(full_ls2, els);
+
EXPECT_THROW(els.extend(short_ls, buf), isc::BadValue);
+
+ EXPECT_TRUE(els.isAbsolute());
+ check_equal(full_ls2, els);
}
}
More information about the bind10-changes
mailing list