Opened 7 years ago

Closed 7 years ago

#2417 closed task (fixed)

Standard option definitions initialization for DHCPv6.

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20121115
Component: dhcp6 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

Each standard option must have a corresponding OptionDefinition? object instance. This task includes storage creation for DHCPv6 standard option definitions. Due to time constraints, this task involves creation of most critical standard options only.

Subtickets

Change History (7)

comment:1 Changed 7 years ago by marcin

  • Summary changed from Standard option definitions initialization or DHCPv6. to Standard option definitions initialization for DHCPv6.

comment:2 Changed 7 years ago by marcin

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

comment:3 Changed 7 years ago by marcin

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

The definitions of most critical options are created and put into storage. If there is a need to initialize option that has no definition object initialized the generic option is returned.

Please review.

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

  • Owner changed from UnAssigned to marcin

Reviewed commit 2c70372efd0b0ddb8059d5bbe9eb195f024d4df8

src/bin/dhcp6/config_parser.cc
createOption(): Need a comment before the block of close starting with the call to getOptionDefs. (I'd also suggest adding a comment to the preceding code block, the one that gets the string parameter "data": it's not clear why the data should be a string of hexadecimal digits.)

createOption(): Also not clear why, if the number of definitions is zero, an option is created. Shouldn't this be an error?

createOption(): if getFactory() "should never return NULL", check that with an assert() call (include "cassert", not "assert.h").

src/bin/dhcp6/dhcp6_messages.mes
Suggest replacing this message with two messages:

DHCP6_NO_SUBNET_DEF_OPT failed to find subnet for address %1 when adding default options
This warning message indicates that when attempting to add default options to a response,
the server found that it was not configured to support the subnet from which the DHCPv6
request was received.  The packet has been ignored.

DHCP6_NO_SUBNET_REQ_OPT failed to find subnet for address %1 when adding requested options
This warning message indicates that when attempting to add requested options to a response,
the server found that it was not configured to support the subnet from which the DHCPv6
request was received.  The packet has been ignored.

(See below for reason.)

src/bin/dhcp6/dhcp6_srv.cc
DHCP6_NO_SUBNET_FOR_ADDRESS: two places where this message is issued, which goes against the philospohy of the BIND 10 messaging system. Suggest we have two separarate message (listed above) for the two places where the warning is issued.

append{Default,Requested}Options(): presumably it is only because this is an early implementation that options are associated with the subnet. Are there global options?

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
solicitBasic test: avoid having spaces after an opening parenthesis and before a closing one (this appears in a number of gtest ASSERT_xxx and EXPECT_xxx).

src/lib/dhcp/libdhcp++.cc
initStdOptionDefs6(): In the "switch" statement, the D60_STATUS_CODE case should end with a "break". Also, the "default:" does not need a "break" - it can be empty (although add a comment to note that the default case has been explicitly considered and that no default processing is needed).

src/lib/dhcp/option_definition.h
typo: "oreder"

Suggest that the header for the OptionDefContainer typedef is extended to clarify that there can be multiple elements for a given option code index. (It's clear once you note that a hashed_non-unique index is used, but
making it explicitly clear wouldn't hurt.)

src/lib/dhcp/tests/libdhcp++_unittest.cc
testInitOptionDefs6: That's quite a block of solid code: would prefer a few blank lines and a brief comment indicating what is being checked and why.

initStdOptionDefs: trivial point - why not layout the tests in alphabetical order of the option code?

src/lib/dhcp/tests/option_definition_unittest.cc
factoryBinary test: comments explaining what is being tested (and why) would be useful.

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

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit 2c70372efd0b0ddb8059d5bbe9eb195f024d4df8

src/bin/dhcp6/config_parser.cc
createOption(): Need a comment before the block of close starting with the call to getOptionDefs. (I'd also suggest adding a comment to the preceding code block, the one that gets the string parameter "data": it's not clear why the data should be a string of hexadecimal digits.)

Comment added.

createOption(): Also not clear why, if the number of definitions is zero, an option is created. Shouldn't this be an error?

I added the comment. In the future an error will be issued if option definition is not found. Due to my minimalistic approach to implement definitions for only those options that are critical for now, we can't emit error for now.

createOption(): if getFactory() "should never return NULL", check that with an assert() call (include "cassert", not "assert.h").

Added.

src/bin/dhcp6/dhcp6_messages.mes
Suggest replacing this message with two messages:

DHCP6_NO_SUBNET_DEF_OPT failed to find subnet for address %1 when adding default options
This warning message indicates that when attempting to add default options to a response,
the server found that it was not configured to support the subnet from which the DHCPv6
request was received.  The packet has been ignored.

DHCP6_NO_SUBNET_REQ_OPT failed to find subnet for address %1 when adding requested options
This warning message indicates that when attempting to add requested options to a response,
the server found that it was not configured to support the subnet from which the DHCPv6
request was received.  The packet has been ignored.

(See below for reason.)

src/bin/dhcp6/dhcp6_srv.cc[[BR]

Changed.

DHCP6_NO_SUBNET_FOR_ADDRESS: two places where this message is issued, which goes against the philospohy of the BIND 10 messaging system. Suggest we have two separarate message (listed above) for the two places where the warning is issued.

Uh! I was not aware of this philosophy. But it is good that I am aware from now on. :-)

append{Default,Requested}Options(): presumably it is only because this is an early implementation that options are associated with the subnet. Are there global options?

Options are stored for subnets (in Subnet objets). There is a way to specify option values globally and they are inherited by subnets in case the particular option value is not overriden in a subnet.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
solicitBasic test: avoid having spaces after an opening parenthesis and before a closing one (this appears in a number of gtest ASSERT_xxx and EXPECT_xxx).

I removed all extra spaces in this file.

src/lib/dhcp/libdhcp++.cc
initStdOptionDefs6(): In the "switch" statement, the D60_STATUS_CODE case should end with a "break". Also, the "default:" does not need a "break" - it can be empty (although add a comment to note that the default case has been explicitly considered and that no default processing is needed).

Fixed.

src/lib/dhcp/option_definition.h
typo: "oreder"

Fixed.

Suggest that the header for the OptionDefContainer typedef is extended to clarify that there can be multiple elements for a given option code index. (It's clear once you note that a hashed_non-unique index is used, but
making it explicitly clear wouldn't hurt.)

Extended comments either in the typdef header or at the index.

src/lib/dhcp/tests/libdhcp++_unittest.cc
testInitOptionDefs6: That's quite a block of solid code: would prefer a few blank lines and a brief comment indicating what is being checked and why.

Comments added.

initStdOptionDefs: trivial point - why not layout the tests in alphabetical order of the option code?

They are laid in the order or actual option codes. This is intentional and it helps me to identify which options are already processed in the code and which not. I match it with options listed here http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml for this.

src/lib/dhcp/tests/option_definition_unittest.cc
factoryBinary test: comments explaining what is being tested (and why) would be useful.

Added.


Since we initialize just a limited set of option definitions I suggest that we postpone changelog entry until it is fully implemented.

comment:6 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

src/bin/dhcp6/config_parser.cc
No include of "cassert", although it does not appear to be needed as something else appears to include it. No change needed.

src/lib/dhcp/option_definition.h
Is the tag "<b>" needed in the header for OptionDefContainer?

OK to merge, although if you are planning on adding a ChangeLog entry, that should be reviewed.

comment:7 Changed 7 years ago by marcin

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

Merged.

Note: See TracTickets for help on using tickets.