Opened 7 years ago

Closed 7 years ago

#2237 closed task (fixed)

Pool/Lease Configuration - IPv4

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20121018
Component: ~dhcpconf(obsolete) 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

Implement the extraction of data from the configuration database and use of that data to set up parameters in the DHCP server and the pool/lease database. The ticket covers:

  • Definition of the structure of the parameter hierarchy in the .spec file.
  • Handling the call made when the parameters are changed. (This covers both the initial configuration when the code starts up, and the re-configuration when the parameters are changed.)
  • User documentation of the parameters that can be set

This task covers the IPv4 server only, and only those parameters required for the end of 2012 milestone. Additional parameters will be added as the code is extended.

Subtickets

Change History (7)

comment:1 Changed 7 years ago by tomek

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

comment:2 Changed 7 years ago by tomek

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

DHCP Configuration Manager is now able to store IPv4 subnets and pools. It does not cover parsing actual configuration. That is done as part of #2270 that is planned immediately after dealing with its v6 counterpart (#2269).

The code is ready for review.

comment:3 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 46c03d26e27417bdce132b90eef18686443bd5a7

General
In a lot of calls, "prefix length" is passed as a uint8_t. Unless there is a compelling reason for this, I would have thought that an "unsigned int" - using the natural word length of the machine - would be faster.

src/lib/dhcp/addr_utilities.cc
The .h file doesn't seem to have been updated with the function definitions.

Instead of using "static", put masks and bitMask into the anonymous namespace.

firstAddrInPrefix4: require spaces around the operator ">". Also spaces around "-" in "32-len".

{first,last}AddrInPrefix4: In both cases where "mask" is referenced, it is always "mask[32 - len]". Why not reverse the order of the elements of in the initialization of "mask" and just refer to it as "mask[len]"?

There are no unit tests for the new xxx4 functions.

src/lib/dhcp/cfgmgr.h
There is a definition for removeSubnets4() but not for removeSubnets6().

Similarly, there is a getSubnet6() by interface ID, but not a getSubnet4() by interface ID.

src/lib/dhcp/tests/cfgmgr_unittest.cc
test subnet4: Would add a comment to the test recording (a) adding more subnets and checking them and (b) attempting to obtain an address not in the subnet.

test subnet4: Would also test what happens if you attempt to add a subnet that is already present.

(Same comments go for the subnet6 test.)

comment:5 in reply to: ↑ 4 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit 46c03d26e27417bdce132b90eef18686443bd5a7

General
In a lot of calls, "prefix length" is passed as a uint8_t. Unless there is a compelling reason for this, I would have thought that an "unsigned int" - using the natural word length of the machine - would be faster.

In general, I try to use the minimal type that is able to hold maximum valid value. In this context, the ranges are 0-32 or 0-128, so both can be conveyed in uint8_t. You seem to prefer uint32_t. I've asked in bind10 chatroom, and got response from vorner slightly preferring unsigned int (because internal arithmetics in C/C++ must be done on types at least as large as int) and Mark Andrews preferring uint8_t. Vorner later added that for consistency reasons it makes sense to stick with uint8_t.

This gives the result of 2 vs 2, with a slight preference to uint8_t. I decided to stick with uint8_t, because:

  • the alternative would mean that we need to do a lot of changed with zero usability gains
  • from storage perspective uint8_t is better. Think about million users provided with prefixes.
  • using uint8_t for storage and uint32_t for calculations would be ugly and is error prone

If you disagree with those arguments, please submit a new ticket for that work. Keep in mind that it affects most of the DHCP code, so it is much bigger than just changing parameters in addr_utilities

src/lib/dhcp/addr_utilities.cc
The .h file doesn't seem to have been updated with the function definitions.

That's by design. Those new functions are private. The external interface is done via generic functions that accept both protocol families. I don't want anyone to call v4 and v6 functions directly as they lack sanity checks. Those sanity checks are done in generic, public versions.

Added comments that explain that design.

Instead of using "static", put masks and bitMask into the anonymous namespace.

Done. Also renamed bitMask to bitMask6 and masks to bitMask4.

firstAddrInPrefix4: require spaces around the operator ">". Also spaces around "-" in "32-len".

Done.

{first,last}AddrInPrefix4: In both cases where "mask" is referenced, it is always "mask[32 - len]". Why not reverse the order of the elements of in the initialization of "mask" and just refer to it as "mask[len]"?

I wrote the array in that order and only later realized how I'm going to use it. It doesn't make much sense, so I've reverted the order as your suggested.

There are no unit tests for the new xxx4 functions.

Implemented new tests.

src/lib/dhcp/cfgmgr.h
There is a definition for removeSubnets4() but not for removeSubnets6().

That is done as part of #2269 work. I don't want to fix it in this ticket to make the merge more complicated than necessary. Added comment 6 in #2269.

Similarly, there is a getSubnet6() by interface ID, but not a getSubnet4() by interface ID.

Aha! Because there is no interface-id concept in DHCPv4 :)

src/lib/dhcp/tests/cfgmgr_unittest.cc
test subnet4: Would add a comment to the test recording (a) adding more subnets and checking them and (b) attempting to obtain an address not in the subnet.

test subnet4: Would also test what happens if you attempt to add a subnet that is already present.

(Same comments go for the subnet6 test.)

Such a test would fail. See @todo in CfgMgr::addSubnet4(). Also added #2346.

comment:6 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 5e39872cc6dc260c91596ee31f1601dbf53de503

src/lib/dhcp/addr_utilities.cc
The .h file doesn't seem to have been updated with the function definitions.

That's by design. Those new functions are private. The external interface is done via generic functions that accept both protocol families. I don't want anyone to call v4 and v6 functions directly as they lack sanity checks. Those sanity checks are done in generic, public versions.

In that case, that should be either made static or put within the anonymous namespace in the file (so that they can't be called from outside the file).

Once that is corrected, the code can be merged: I don't need to see it again.

comment:7 Changed 7 years ago by tomek

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

Private v4 and v6 functions are now in anonymous namespace. Code merged.

Note: See TracTickets for help on using tickets.