Commit 43ad2f49 authored by Thomas Markwalder's avatar Thomas Markwalder
Browse files

[5058] - Addressed review comments

    Fixed typos and added decline lease checks for DHCPv6

src/lib/dhcp/duid.h
src/lib/dhcp/duid.cc
src/lib/dhcp/tests/duid_unittest.cc
    - Replaced DUID::generateEmpty() with DUID::EMPTY() which returns
    a constant reference to a static DUID instance.  This facilitates
    comparisions.
    - Updated TEST(DuidTest,empty) accordingly

src/lib/dhcpsrv/csv_lease_file6.cc
    CSVLeaseFile6::next() - added test to permit the Empty DUID
    only if state is STATE_DECLINED.

src/lib/dhcpsrv/lease.cc
    Lease6::decline() - modified to use DUID::EMPTY().

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
    Fixed Typos

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
    TEST_F(CSVLeaseFile6Test, declinedLeaseTest) - new test to
    verify proper handling of declined leases
parent 939e65e3
......@@ -62,16 +62,11 @@ DUID::fromText(const std::string& text) {
return (DUID(binary));
}
DuidPtr
DUID::generateEmpty() {
// Technically this is a one byte DUID with value of 0. However, we do have
// a number of safety checks against invalid duids (too long or empty) and
// we should keep them. Therefore "empty" means a single byte with value of 0.
std::vector<uint8_t> empty_duid(1,0);
DuidPtr duid(new DUID(empty_duid));
return (duid);
const DUID&
DUID::EMPTY() {
static std::vector<uint8_t> empty_duid(1,0);
static DUID empty(empty_duid);
return (empty);
}
std::string DUID::toText() const {
......
......@@ -61,17 +61,16 @@ class DUID {
/// @return A reference to a vector holding a DUID.
const std::vector<uint8_t>& getDuid() const;
/// @brief Returns an instance of empty DUID
/// @brief Defines the constant "empty" DUID
///
/// In general, empty DUID is not allowed. The only case where it is really
/// valid is to designate declined IPv6 Leases. We have a broad assumption
/// that the Lease->duid_ must always be set. However, declined lease
/// doesn't have any DUID associated with it. Hence we need a way to
/// indicate that fact.
///
/// @return a smart pointer to an empty DUID
static DuidPtr generateEmpty();
//
/// @return reference to the static constant empty DUID
static const DUID& EMPTY();
/// @brief Returns the DUID type
DUIDType getType() const;
......
......@@ -160,7 +160,7 @@ TEST(DuidTest, toText) {
// This test verifies that empty DUID returns proper value
TEST(DuidTest, empty) {
DuidPtr empty;
EXPECT_NO_THROW(empty = DUID::generateEmpty());
EXPECT_NO_THROW(empty.reset(new DUID(DUID::EMPTY())));
// This method must return something
ASSERT_TRUE(empty);
......@@ -168,6 +168,13 @@ TEST(DuidTest, empty) {
// Ok, technically empty is not really empty, it's just a single
// byte with value of 0.
EXPECT_EQ("00", empty->toText());
EXPECT_TRUE(*empty == DUID::EMPTY());
uint8_t data1[] = {0, 1, 2, 3, 4, 0xff, 0xfe};
DUID duid(data1, sizeof(data1));
EXPECT_FALSE(duid == DUID::EMPTY());
}
// This test checks if the comparison operators are sane.
......
// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -94,7 +94,11 @@ CSVLeaseFile6::next(Lease6Ptr& lease) {
lease->fqdn_rev_ = readFqdnRev(row);
lease->hostname_ = readHostname(row);
lease->state_ = readState(row);
if ((*lease->duid_ == DUID::EMPTY())
&& lease->state_ != Lease::STATE_DECLINED) {
isc_throw(isc::BadValue, "The Empty DUID is"
"only valid for declined leases");
}
} catch (std::exception& ex) {
// bump the read error count
++read_errs_;
......
......@@ -275,7 +275,7 @@ Lease6::getDuidVector() const {
void
Lease6::decline(uint32_t probation_period) {
hwaddr_.reset();
duid_ = DUID::generateEmpty();
duid_.reset(new DUID(DUID::EMPTY()));
t1_ = 0;
t2_ = 0;
preferred_lft_ = 0;
......
......@@ -146,7 +146,7 @@ TEST_F(CSVLeaseFile4Test, parse) {
}
// Second lease is malformed - HW address is empty when state
// is not delcined.
// is not declined.
{
SCOPED_TRACE("Second lease malformed");
EXPECT_FALSE(lf.next(lease));
......@@ -386,7 +386,7 @@ TEST_F(CSVLeaseFile4Test, downGrade) {
}
}
// Verifies that leases with no hardware address or only permitted
// Verifies that leases with no hardware address are only permitted
// if they are in the declined state.
TEST_F(CSVLeaseFile4Test, declinedLeaseTest) {
io_.writeFile("address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
......@@ -394,12 +394,10 @@ TEST_F(CSVLeaseFile4Test, declinedLeaseTest) {
"192.0.2.1,,,200,200,8,1,1,host.example.com,0\n"
"192.0.2.1,,,200,200,8,1,1,host.example.com,1\n");
// Lease file should open and report as needing downgrade.
CSVLeaseFile4 lf(filename_);
ASSERT_NO_THROW(lf.open());
EXPECT_FALSE(lf.needsConversion());
EXPECT_EQ(util::VersionedCSVFile::CURRENT,
lf.getInputSchemaState());
EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState());
Lease4Ptr lease;
{
......
......@@ -451,6 +451,48 @@ TEST_F(CSVLeaseFile6Test, downGrade) {
}
}
// Verifies that leases with no DUID are invalid, and that leases
// with the "Empty" DUID (1 byte duid = 0x0) are valid only when
// in the declined state.
TEST_F(CSVLeaseFile6Test, declinedLeaseTest) {
io_.writeFile("address,duid,valid_lifetime,expire,subnet_id,"
"pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
"fqdn_rev,hostname,hwaddr,state\n"
"2001:db8:1::1,00,"
"200,200,8,100,0,7,0,1,1,host.example.com,,0\n"
"2001:db8:1::1,,"
"200,200,8,100,0,7,0,1,1,host.example.com,,0\n"
"2001:db8:1::1,00,"
"200,200,8,100,0,7,0,1,1,host.example.com,,1\n");
CSVLeaseFile6 lf(filename_);
ASSERT_NO_THROW(lf.open());
EXPECT_FALSE(lf.needsConversion());
EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState());
Lease6Ptr lease;
{
SCOPED_TRACE("\"Empty\" DUID and not declined, invalid");
EXPECT_FALSE(lf.next(lease));
ASSERT_FALSE(lease);
EXPECT_EQ(lf.getReadErrs(),1);
}
{
SCOPED_TRACE("Missing (blank) DUID and not declined, invalid");
EXPECT_FALSE(lf.next(lease));
ASSERT_FALSE(lease);
EXPECT_EQ(lf.getReadErrs(),2);
}
{
SCOPED_TRACE("\"Empty\" DUID and declined, valid");
EXPECT_TRUE(lf.next(lease));
ASSERT_TRUE(lease);
EXPECT_EQ(lf.getReadErrs(),2);
}
}
/// @todo Currently we don't check invalid lease attributes, such as invalid
/// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment