Opened 6 years ago

Closed 6 years ago

#2596 closed enhancement (fixed)

Tie DHCP V6 server subnet to interface

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130122
Component: dhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The DHCPv6 server opens sockets on all connected interfaces. However, there is no way of associating a particular subnet declaration to a given interface.

Subtickets

Change History (9)

comment:1 Changed 6 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:2 Changed 6 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

Code change has been checked in and is ready for review.

comment:3 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:4 follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit d47e9e008a7eb572f370ab7b5527c8188bb24017

ChangeLog looks ok.

src/bin/dhcp6/config_parser.cc

createSubnet:

Line 1142: The const_iterator could be used instead as the value is read only.

Line 1164: the following condition:

if (iface.length()) {
...
}

could be replaced with:

if (!iface.empty()) {
...
}

as the latter is in general more efficient. Also sometimes the cppcheck complains about the efficiency in such cases.

Line 1165: if the interface name is specified then it is set for a particular subnet being configured:

subnet_->setIface(iface);

The modifier function does not validate the iface. In particular a user may be interested if the interface exists (in case he mistype it). IMO, the configuration parser should report an error at this point if the interface is wrong. Otherwise he is going to go through the configuration commit successfully and then will get cryptic errors or will not get any error at all but the server will misbehave.

src/lib/dhcpsrv/subnet.h
The iface_name parameter in the setIface's header has not been documented.

getIface: should be declared const as it does modify the class members

src/lib/dhcpsrv/tests/subnet_unittest.cc
TEST(Subnet6Test, iface): Trivial thing but it could be probably better to do:

EXPECT_TRUE(subnet.getIface().empty());

It looks more clear than comparison against empty string.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit d47e9e008a7eb572f370ab7b5527c8188bb24017
src/bin/dhcp6/config_parser.cc

createSubnet:
Line 1142: The const_iterator could be used instead as the value is read only.

Done.

Line 1164: the following condition:

if (iface.length()) {
...
}

could be replaced with:

if (!iface.empty()) {
...
}

as the latter is in general more efficient. Also sometimes the cppcheck complains about the efficiency in such cases.

Done.

Line 1165: if the interface name is specified then it is set for a particular subnet being configured:

subnet_->setIface(iface);

The modifier function does not validate the iface. In particular a user may be interested if the interface exists (in case he mistype it). IMO, the configuration parser should report an error at this point if the interface is wrong. Otherwise he is going to go through the configuration commit successfully and then will get cryptic errors or will not get any error at all but the server will misbehave.

Ok. Implemented validation. Also modified the test to pass on systems that do not have eth0 interface.

src/lib/dhcpsrv/subnet.h
The iface_name parameter in the setIface's header has not been documented.

It is documented now.

getIface: should be declared const as it does modify the class members

Done.

src/lib/dhcpsrv/tests/subnet_unittest.cc
TEST(Subnet6Test, iface): Trivial thing but it could be probably better to do:

EXPECT_TRUE(subnet.getIface().empty());

It looks more clear than comparison against empty string.

Done.


comment:6 in reply to: ↑ 5 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

Reviewed commit f3709cc11e26e2bb2a0cfa4f447efce885160276

Replying to marcin:

Line 1165: if the interface name is specified then it is set for a particular subnet being configured:

subnet_->setIface(iface);

The modifier function does not validate the iface. In particular a user may be interested if the interface exists (in case he mistype it). IMO, the configuration parser should report an error at this point if the interface is wrong. Otherwise he is going to go through the configuration commit successfully and then will get cryptic errors or will not get any error at all but the server will misbehave.

Ok. Implemented validation. Also modified the test to pass on systems that do not have eth0 interface.

src/bin/dhcp6/tests/config_parser_unittest.cc
Although it is unlikely, it is possible that someone has named his or her interface nonexisting0. If so, the subnetInterfaceBugus test will fail saying that the configuration was successful while expected failure. It is ok, that the test fails but it could actually provide some better information on failure if it was sanity checked that the bogus interface actually exists. So for example, the test could:

ASSERT_TRUE(IfaceMgr::instance().getIface(bogus_iface_).empty())
    << "The '" << bogus_interface_ << "' exists on this system"
    << " while the test assumes that it doesn't to execute some negative"
    << " testing scenario. Can't continue this test";

The test fails anyway but it may give some more insight into the failure reason.

What do you think?

comment:7 Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

Good idea. It is tricky as the code is in constructor, so we can't use ASSERT there (ASSERT_* from gtest is a macro that returns a value). Added ADD_FAILURE() with appropriate comment.

comment:8 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

I made a minor editorial change to the error message and pushed it.

Please merge.

comment:9 Changed 6 years ago by tomek

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged, pushed, closed.

Note: See TracTickets for help on using tickets.