Skip to content
GitLab
Commit 38d6de2b authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3768] Address review comments.

parent 7ab8a311
Hide whitespace changes
Inline Side-by-side
src/bin/dhcp4/tests/dhcp4_client.cc
View file @ 38d6de2b
......@@ -86,6 +86,54 @@ Dhcp4Client::addRequestedAddress(const asiolink::IOAddress& addr) {
}
}
void
Dhcp4Client::appendClientId() {
if (!context_.query_) {
isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
" when adding Client Identifier option");
}
if (clientid_) {
OptionPtr opt(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER,
clientid_->getClientId()));
context_.query_->addOption(opt);
}
}
void
Dhcp4Client::appendName() {
if (!context_.query_) {
isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
" when adding FQDN or Hostname option");
}
if (fqdn_) {
context_.query_->addOption(fqdn_);
} else if (hostname_) {
context_.query_->addOption(hostname_);
}
}
void
Dhcp4Client::appendPRL() {
if (!context_.query_) {
isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
" when adding option codes to the PRL option");
} else if (!requested_options_.empty()) {
// Include Parameter Request List if at least one option code
// has been specified to be requested.
OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
DHO_DHCP_PARAMETER_REQUEST_LIST));
for (std::set<uint8_t>::const_iterator opt = requested_options_.begin();
opt != requested_options_.end(); ++opt) {
prl->addValue(*opt);
}
context_.query_->addOption(prl);
}
}
void
Dhcp4Client::applyConfiguration() {
Pkt4Ptr resp = context_.response_;
......@@ -152,11 +200,11 @@ void
Dhcp4Client::doDiscover(const boost::shared_ptr<IOAddress>& requested_addr) {
context_.query_ = createMsg(DHCPDISCOVER);
// Request options if any.
includePRL();
appendPRL();
// Include FQDN or Hostname.
includeName();
appendName();
// Include Client Identifier
includeClientId();
appendClientId();
if (requested_addr) {
addRequestedAddress(*requested_addr);
}
......@@ -182,7 +230,7 @@ void
Dhcp4Client::doInform(const bool set_ciaddr) {
context_.query_ = createMsg(DHCPINFORM);
// Request options if any.
includePRL();
appendPRL();
// The client sending a DHCPINFORM message has an IP address obtained
// by some other means, e.g. static configuration. The lease which we
// are using here is most likely set by the createLease method.
......@@ -245,11 +293,11 @@ Dhcp4Client::doRequest() {
}
// Request options if any.
includePRL();
appendPRL();
// Include FQDN or Hostname.
includeName();
appendName();
// Include Client Identifier
includeClientId();
appendClientId();
// Send the message to the server.
sendMsg(context_.query_);
// Expect response.
......@@ -270,20 +318,6 @@ Dhcp4Client::includeClientId(const std::string& clientid) {
}
}
void
Dhcp4Client::includeClientId() {
if (!context_.query_) {
isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
" when adding Client Identifier option");
}
if (clientid_) {
OptionPtr opt(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER,
clientid_->getClientId()));
context_.query_->addOption(opt);
}
}
void
Dhcp4Client::includeFQDN(const uint8_t flags, const std::string& fqdn_name,
Option4ClientFqdn::DomainNameType fqdn_type) {
......@@ -296,39 +330,6 @@ Dhcp4Client::includeHostname(const std::string& name) {
hostname_.reset(new OptionString(Option::V4, DHO_HOST_NAME, name));
}
void
Dhcp4Client::includeName() {
if (!context_.query_) {
isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
" when adding FQDN or Hostname option");
}
if (fqdn_) {
context_.query_->addOption(fqdn_);
} else if (hostname_) {
context_.query_->addOption(hostname_);
}
}
void
Dhcp4Client::includePRL() {
if (!context_.query_) {
isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
" when adding option codes to the PRL option");
} else if (!requested_options_.empty()) {
// Include Parameter Request List if at least one option code
// has been specified to be requested.
OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
DHO_DHCP_PARAMETER_REQUEST_LIST));
for (std::set<uint8_t>::const_iterator opt = requested_options_.begin();
opt != requested_options_.end(); ++opt) {
prl->addValue(*opt);
}
context_.query_->addOption(prl);
}
}
HWAddrPtr
Dhcp4Client::generateHWAddr(const uint8_t htype) const {
......@@ -369,7 +370,6 @@ Dhcp4Client::requestOptions(const uint8_t option1, const uint8_t option2,
requestOption(option3);
}
Pkt4Ptr
Dhcp4Client::receiveOneMsg() {
// Return empty pointer if server hasn't responded.
......
src/bin/dhcp4/tests/dhcp4_client.h
View file @ 38d6de2b
......@@ -375,14 +375,14 @@ private:
/// This function creates an instance of the Client Identifier option
/// if the client identifier has been specified and includes this
/// option in the client's message to the server.
void includeClientId();
void appendClientId();
/// @brief Includes FQDN or Hostname option in the client's message.
///
/// This method checks if @c fqdn_ or @c hostname_ is specified and
/// includes it in the client's message. If both are specified, the
/// @c fqdn_ will be used.
void includeName();
void appendName();
/// @brief Include PRL Option in the query message.
///
......@@ -390,7 +390,7 @@ private:
/// option and adds option codes from the @c requested_options_ to it.
/// It later adds the PRL option to the @c context_.query_ message
/// if it is non-NULL.
void includePRL();
void appendPRL();
/// @brief Simulates reception of the message from the server.
///
......
src/bin/dhcp4/tests/dora_unittest.cc
View file @ 38d6de2b
......@@ -191,6 +191,7 @@ public:
/// lease without a unique identifier.
/// - The client sends the DHCPREQUEST and the server sends the DHCPNAK for the
/// same reason.
/// - The client A renews its address successfully.
///
/// The specific test cases using this test must make sure that one of the
/// provided parameters is an empty string. This simulates the situation where
......@@ -218,7 +219,7 @@ public:
/// identifier of the client A.
/// - Client B sends DHCPDISCOVER.
/// - Server should determine that the client B is not client A, because
/// it is using a different hadrware address or client identifier, even
/// it is using a different hadrware address or client identifier.
/// As a consequence, the server should offer a different address to the
/// client B.
/// - The client B performs the 4-way exchange again, and the server
......@@ -226,6 +227,7 @@ public:
/// than the address used by the client A.
/// - Client B is in the renewing state and it successfully renews its
/// address.
/// - The client A also renews its address successfully.
///
/// @param hwaddr_a HW address of client A.
/// @param clientid_a Client id of client A.
......@@ -569,6 +571,14 @@ DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a,
resp_b = client_b.getContext().response_;
ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_);
// Client A should also be able to renew its address.
client_a.setState(Dhcp4Client::RENEWING);
ASSERT_NO_THROW(client_a.doRequest());
ASSERT_TRUE(client_a.getContext().response_);
resp_b = client_a.getContext().response_;
ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
ASSERT_NE(client_a.config_.lease_.addr_, client_b.config_.lease_.addr_);
}
void
......@@ -612,6 +622,14 @@ DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a,
resp_b = client_b.getContext().response_;
ASSERT_TRUE(resp_b);
ASSERT_EQ(DHCPNAK, static_cast<int>(resp_b->getType()));
// Client A should also be able to renew its address.
client_a.setState(Dhcp4Client::RENEWING);
ASSERT_NO_THROW(client_a.doRequest());
ASSERT_TRUE(client_a.getContext().response_);
resp_b = client_a.getContext().response_;
ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
ASSERT_NE(client_a.config_.lease_.addr_, client_b.config_.lease_.addr_);
}
// This test checks the server behavior in the following situation:
......@@ -630,10 +648,7 @@ DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a,
// than the address used by the client A.
// - Client B is in the renewing state and it successfully renews its
// address.
// - Client B goes to INIT_REBOOT state and asks for the address used
// by the client A.
// - The server sends DHCPNAK to refuse the allocation of this address
// to the client B.
// - Client A also renews its address successfully.
TEST_F(DORATest, twoAllocationsOverlap1) {
twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34",
"02:02:03:03:04:04", "12:34");
......@@ -659,6 +674,7 @@ TEST_F(DORATest, twoAllocationsOverlap2) {
// lease without a unique identifier.
// - The client sends the DHCPREQUEST and the server sends the DHCPNAK for the
// same reason.
// - Client A renews its address successfully.
TEST_F(DORATest, oneAllocationOverlap1) {
oneAllocationOverlapTest("01:02:03:04:05:06", "12:34",
"01:02:03:04:05:06", "");
......
Supports Markdown
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