Opened 7 years ago

Closed 7 years ago

#2314 closed task (complete)

Namespace encapsulation by Option

Reported by: marcin Owned by: marcin
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 (last modified by stephen)

This task involves creating a "link" between OptionDefinition and available namespaces. In other words, we need to extend the OptionDefinition implementation to store the namespace(s) the particular option belongs to.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by stephen

  • Description modified (diff)
  • Milestone changed from DHCP 2012 to Sprint-DHCP-20121018

comment:2 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20121115 to DHCP 2012

comment:3 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20121213

comment:4 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20130103 to DHCP Outstanding Tasks

comment:5 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130122

comment:6 Changed 7 years ago by marcin

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

comment:7 Changed 7 years ago by marcin

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

The ticket is now ready for a review. Either custom-defined options or some of the standard options can encapsulate option space containing other options. The option space to be encapsulated is specified within an option definition. For standard options, the encapsulated option space name is specified for a limited number of options (e.g. vendor opts).

comment:8 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to marcin

src/bin/dhcp4/config_parser.cc
appendSubOptions: if the pointer is invalid, should an exception be thrown?

appendSubOptions: s/option definition fo the particular/option definition for the particular/

src/bin/dhcp6/config_parser.cc
Same comments as for the V4 version.

src/lib/dhcp/option_definition.cc
In the third constructor (starting at line 63), can't you initialize the value of type_ to OptionDataTypeUtil::getData(type) instead of setting it in the constructor?

src/lib/dhcp/option_definition.h
Most BIND 10 code, when returning a string, returns a string by value. I suggest we do the same here. (The argument is that a (futurte) caller would be able to set up a reference to an internal member variable, which would disappear when the object goes away.) As an asie, I'm not sure that returning a reference in this case gives us much. Compilers are allowed to do return value optimisation, so the overhead should be small.

Copyright Notices
Need to update the copyright date to 2013 in:

  • src/lib/dhcp/option_custom.h
  • src/lib/dhcp/option_definition.cc
  • src/lib/dhcp/tests/option_definition_unittest.cc
  • src/lib/dhcp/tests/libdhcp++_unittest.cc

Documentation
The documentation can be added as part of #264.

comment:10 in reply to: ↑ 9 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

src/bin/dhcp4/config_parser.cc
appendSubOptions: if the pointer is invalid, should an exception be thrown?

For Config Parsers I used to use asserts to guard against programming error. In fact, the NULL pointer in the appendSubOptions functions is also a programming error because in theory the NULL pointer shouldn't be stored in the container. If there is one, this is a serious error. For this reason I added an assert.

appendSubOptions: s/option definition fo the particular/option definition for the particular/

Fixed.

src/bin/dhcp6/config_parser.cc
Same comments as for the V4 version.

Fixed.

src/lib/dhcp/option_definition.cc
In the third constructor (starting at line 63), can't you initialize the value of type_ to OptionDataTypeUtil::getData(type) instead of setting it in the constructor?

Moved to the constructor initialization list.

src/lib/dhcp/option_definition.h
Most BIND 10 code, when returning a string, returns a string by value. I suggest we do the same here. (The argument is that a (futurte) caller would be able to set up a reference to an internal member variable, which would disappear when the object goes away.) As an asie, I'm not sure that returning a reference in this case gives us much. Compilers are allowed to do return value optimisation, so the overhead should be small.

Fair enough. Now, functions return strings by value.

Copyright Notices
Need to update the copyright date to 2013 in:

  • src/lib/dhcp/option_custom.h
  • src/lib/dhcp/option_definition.cc
  • src/lib/dhcp/tests/option_definition_unittest.cc
  • src/lib/dhcp/tests/libdhcp++_unittest.cc

Updated.

Documentation
The documentation can be added as part of #264.

Was that #2642?

Proposed ChangeLog entry:

XXX.	[func]		marcin
	The encapsulated option space name can be specified for
	a DHCP option. It comprises sub-options being sent within
	an option that encapsulates this option space.
	(Trac #2314, git abcdef)

comment:11 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Was that #2642?

Yes :-/.

All OK (including the ChangeLog) - please merge.

comment:12 Changed 7 years ago by marcin

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

Merged to master with the commit 27e6119093723a1e46a239ec245a8b4b10677635

Note: See TracTickets for help on using tickets.