Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#3927 closed enhancement (complete)

Provide default values for parameters defining option definitions

Reported by: marcin Owned by: fdupont
Priority: medium Milestone: Kea1.0-beta
Component: ~dhcpconf(obsolete) Version: git
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

In order to create the option definition (a.k.a. option format), one must specify the "option-def" map in the configuration file. This map comprises a bunch of parameters and currently all those parameters must be specified, even if they are left blank. For the user's convenience, some of those parameters should be optional and the default values should be provided.

For example: if the option type is not a record, there is no senses to specify the "record-types" parameter for it.

Subtickets

Change History (11)

comment:1 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from medium to low

comment:2 Changed 4 years ago by marcin

  • Priority changed from low to medium

Set medium priority as per 1.0 ticket scrub on 07/31/2015.

comment:3 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted
  • Summary changed from Provide default values for parameters definining option definitions to Provide default values for parameters defining option definitions

comment:4 Changed 4 years ago by fdupont

I looked at the code and it uses getOptionalParam() for optional params so IMHO the doc must be fixed but not the code. BTW option-data is more flexible than what the doc describes too.

comment:5 Changed 4 years ago by fdupont

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

I removed a lot of extra lines in option-def and some in option-data unit tests. I tried to fix the guide too. Note I added no new code as I didn't see something really missing...
Ready for review.

comment:6 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

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

  • Owner changed from marcin to fdupont

Reviewed up to commit f5c8bd14f4aac4c4f69131529ff9a6a3aaf49695

Please provide a ChangeLog entry. I suggest "User Guide: parameters having default values may be omitted in the option definitions".

doc/guide/dhcp6-srv.xml
Copy-paste error "dhcp4" in "The default space is dhcp4."

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
minimalOptionDefTest: I think it is a bit misleading to say that this is a minimal configuration for option definition, because you provide the "state" explicitly. Either this test or some other test should check that the space can be omitted and the definition is added to the default space, i.e. "dhcp4" or "dhcp6". I realize that there are issues with the shortage of available dhcp4 option code, but I don't see this issue for "dhcp6" option space. So perhaps, use some high value of the option code in the "dhcp6" space for this test.

minimalOptionDataTest: Shouldn't the comment "Verify that the option definition is correct" rather be "Verify that the option data is correct"

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

Reviewed up to commit f5c8bd14f4aac4c4f69131529ff9a6a3aaf49695

Please provide a ChangeLog entry. I suggest "User Guide: parameters having default values may be omitted in the option definitions".

=> fine.

doc/guide/dhcp6-srv.xml
Copy-paste error "dhcp4" in "The default space is dhcp4."

=> fixed (I searched for 4's in the whole DHCPv6 guide too).

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
minimalOptionDefTest: I think it is a bit misleading to say that this is a minimal configuration for option definition, because you provide the "space" explicitly.

=> as the comment says it is a copy of the basic test where all optional values set to their default were removed.

Either this test or some other test should check that the space can be omitted and the definition is added to the default space, i.e. "dhcp4" or "dhcp6".

=> in fact it was done in the dhcp4 and dhcp6 unit tests.

I realize that there are issues with the shortage of available dhcp4 option code, but I don't see this issue for "dhcp6" option space. So perhaps, use some high value of the option code in the "dhcp6" space for this test.

=> I am adding a defaultSpace test with a comment explaining the default universe is V6.

minimalOptionDataTest: Shouldn't the comment "Verify that the option definition is correct" rather be "Verify that the option data is correct"

=> I copy the comment from basicOptionalDataTest so should I fix it in both tests?

comment:9 in reply to: ↑ 8 Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

Replying to marcin:

Reviewed up to commit f5c8bd14f4aac4c4f69131529ff9a6a3aaf49695

Please provide a ChangeLog entry. I suggest "User Guide: parameters having default values may be omitted in the option definitions".

=> fine.

doc/guide/dhcp6-srv.xml
Copy-paste error "dhcp4" in "The default space is dhcp4."

=> fixed (I searched for 4's in the whole DHCPv6 guide too).

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
minimalOptionDefTest: I think it is a bit misleading to say that this is a minimal configuration for option definition, because you provide the "space" explicitly.

=> as the comment says it is a copy of the basic test where all optional values set to their default were removed.

Either this test or some other test should check that the space can be omitted and the definition is added to the default space, i.e. "dhcp4" or "dhcp6".

=> in fact it was done in the dhcp4 and dhcp6 unit tests.

I realize that there are issues with the shortage of available dhcp4 option code, but I don't see this issue for "dhcp6" option space. So perhaps, use some high value of the option code in the "dhcp6" space for this test.

=> I am adding a defaultSpace test with a comment explaining the default universe is V6.

ok. And instead of

ASSERT_TRUE(rcode_ == 0);

you could use

ASSERT_EQ(0, rcode_);

minimalOptionDataTest: Shouldn't the comment "Verify that the option definition is correct" rather be "Verify that the option data is correct"

=> I copy the comment from basicOptionalDataTest so should I fix it in both tests?

Yes.

Once you correct those minor issues, please merge the branch.

comment:10 Changed 4 years ago by fdupont

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

Merged. Closing.

comment:11 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.