Opened 7 years ago

Closed 7 years ago

#2637 closed defect (fixed)

DHCP code tidy-up for the release in January 2013

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

There are a few things that should be cleaned up in the current code:

  • OptionCustom should take pointer to the OptionDefinition rather than a const reference in the constructor. Otherwise the definition may invalidate at some point during the life of the option object.
  • In the config_parser.cc there are a couple of @todos to be completed after a merge of #2313. The merge has been completed already so the appropriate dependent implementation should be added.
  • The DHCP configuration handler is currently using the ''partial'' configuration to configure a server. Since we have dependencies between parsers we rather have to use the full configuration string to make sure that the dependencies are satisfied. Otherwise the partial configuration may just trigger modifications to some elements but not to the elements which values depend on them.

-...

Subtickets

Change History (11)

comment:1 Changed 7 years ago by stephen

Some other trivial issues that can be tidied up here:

  • (src/bin/dhcp6/dhcp6_srv.cc) The logging call for message DHCP6_RESPONSE_DATA should convert the type code logged in the message to an "int" - it's currently a unit8_t, which gets output as an unprintable character.
  • (src/lib/dhcp/pkt6.cc) Pkt6::toText() - the type code should be converted to an "int" before output (same reasons as above).
  • (src/lib/dhcp/pkt4.cc) Pkt4::toText() - use "static_cast<int>()" instead of "int()" for the conversion.

comment:2 Changed 7 years ago by marcin

Additional trivial issues include:

comment:3 Changed 7 years ago by marcin

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

comment:4 Changed 7 years ago by marcin

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

The summary of changes:

  • Fixed configuration of the V4 and V6 servers:
    • Server did not get the configuration on its startup until some change has been made to the configuration.
    • Config handler now uses full configuration merged with the new configuration passed to it, which should satisfy the dependencies between various parsers.
  • Minor fixes to logging, i.e. cast uint8_t to int to avoid printing of unreadable chars
  • Fixed doxygen and clang static analyzer issues
  • Fixed typos in dhcpX.spec files
  • Enabled setting FQDN for options - ommision in one of the previous tickets
  • Merged !Option::pack4() and !Option::pack6()

Most of the changes made had been introduced as a result of basic manual tests of DHCPv4 and DHCPv6 servers. The test involved installation of a server, running bindctl and attempting to set various parameters, firing the dhclient (or dhclient and relay for V4) and checking whether the lease has been granted and various options returned to the client.

Eventually it has been decided NOT to replace the reference to an OptionDefinition to the pointer when creating OptionCustom object. The copy constructor inside OptionCustom will not let an OptionDefinition object to invalidate.

There is one outstanding issue that has been discovered during testing. That is, the configuration parsers do not handle default values very well. This specifically refers to setting option values and option definitions. For example, the ''dhcp4'' is the default option space for each option. However, when leaving the default value the parser will simply receive partial configuration (that excludes space name). This in turn will cause an error message that the ''space'' parameter is missing. For this reason it is necessary to implement some conditions in option parsers that set default value for various parameters when they are not received from the Config Manager. The #2646 has been submitted to resolve this issue.

comment:5 Changed 7 years ago by marcin

  • Status changed from assigned to reviewing

comment:6 Changed 7 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:7 follow-up: Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

src/bin/dhcp4/config_parser.cc
createOption() verifies if the option_name contains spaces. Why this specific case is checked here, but all other checks are done in OptionSpace::validateName?

src/bin/dhcp4/ctrl_dhcp4_srv.cc
establishSession(): Dumy => Dummy.

Please update (or remove) the comment starting with:
We initially create ModuleCCSession() without configHandler

The DHCP4_CONFIG_UPDATE log entry will print out full_config which ironically does not include full_config. merged_config should be used instead.

On a related note, I think we should have a discussion with DNS team about how this configuration works. I'm uncomfortable with the getFullConfig not returning a full config. Perhaps it should be renamed to getComittedConfig? Or better yet it should be possible to easy obtain both the diff (essentially how it currently works) and the full merged config. Anyway, it's not something that should be done in this ticket.

src/bin/dhcp6/config_parser.cc
createOption() the same comment about checking against space as in its v4 counterpart.

src/lib/dhcp/option.cc
Please extend the comments a bit. Something like this will do the trick:

-    // Write a header.
+    // Write a header (protocol dependent - 2 bytes for DHCPv4 and 4 for DHCPv6)
     packHeader(buf);
-    // Write data.
+    // Write data (the same for both protocols)

src/lib/dhcp/option.h
Comment for pack() is no longer true. This method does not return a pointer anymore.

src/lib/dhcp/option_definition.cc
OptionDefinition::writeToBuffer()
There are no tests for adding FQDN record. If it is time consuming to add one, please add @todo in the test code instead.

comment:8 Changed 7 years ago by tomek

I also created ticket #2651 for the problem is spec files. Although this particular issue is now fixed, we still don't have unit-tests that verify if our spec files are sane.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

src/bin/dhcp4/config_parser.cc
createOption() verifies if the option_name contains spaces. Why this specific case is checked here, but all other checks are done in OptionSpace::validateName?

The OptionSpace::validateName is validating option space name while the createOption checked the option name for emptiness. These are two different things being validated. In any case I added a static function OptionDefinition::validateName that is now used to validate option name.

src/bin/dhcp4/ctrl_dhcp4_srv.cc
establishSession(): Dumy => Dummy.

Fixed.

Please update (or remove) the comment starting with:
We initially create ModuleCCSession() without configHandler

Updated.

The DHCP4_CONFIG_UPDATE log entry will print out full_config which ironically does not include full_config. merged_config should be used instead.

Good catch. Fixed.

On a related note, I think we should have a discussion with DNS team about how this configuration works. I'm uncomfortable with the getFullConfig not returning a full config. Perhaps it should be renamed to getComittedConfig? Or better yet it should be possible to easy obtain both the diff (essentially how it currently works) and the full merged config. Anyway, it's not something that should be done in this ticket.

Maybe, as you say, it should be discussed elsewhere.

src/bin/dhcp6/config_parser.cc
createOption() the same comment about checking against space as in its v4 counterpart.

Fixed.

src/lib/dhcp/option.cc
Please extend the comments a bit. Something like this will do the trick:

-    // Write a header.
+    // Write a header (protocol dependent - 2 bytes for DHCPv4 and 4 for DHCPv6)
     packHeader(buf);
-    // Write data.
+    // Write data (the same for both protocols)

Extended comments as you suggested.

src/lib/dhcp/option.h
Comment for pack() is no longer true. This method does not return a pointer anymore.

Removed the section about returning a pointer.

src/lib/dhcp/option_definition.cc
OptionDefinition::writeToBuffer()
There are no tests for adding FQDN record. If it is time consuming to add one, please add @todo in the test code instead.

I added two tests for FQDNs.

Proposed ChangeLog entry:

XXX.	[bug]		vorner
	Fixed DHCP servers configuration whereby the server did not
	receive a configuration stored in the database on its startup.
	Also, the configuration handler function now uses full configuration
	instead of partial to configure the server. This guarantees that
	dependencies between various configuration parameters are
	fulfilled.

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

  • Owner changed from tomek to marcin

Replying to marcin:

Replying to tomek:

src/bin/dhcp4/config_parser.cc
createOption() verifies if the option_name contains spaces. Why this specific case is checked here, but all other checks are done in OptionSpace::validateName?

The OptionSpace::validateName is validating option space name while the createOption checked the option name for emptiness. These are two different things being validated. In any case I added a static function OptionDefinition::validateName that is now used to validate option name.

It's almost good now, but the second comment (line 767) is invalid:

// Check that the option name has been specified and that it is
// valid.

It is option space, not option name here.

Tests look good. Usage of typeid is quite clever.

Proposed ChangeLog entry:

XXX.	[bug]		vorner
	Fixed DHCP servers configuration whereby the server did not
	receive a configuration stored in the database on its startup.
	Also, the configuration handler function now uses full configuration
	instead of partial to configure the server. This guarantees that
	dependencies between various configuration parameters are
	fulfilled.

Make sure you include also Trac number and git commit-id. Otherwise it looks good, mr Vorner. ;-)

comment:11 Changed 7 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Type changed from task to defect

Merged to master with the commit 91aa998226f1f91a232f2be59a53c9568c4ece77.

Note: See TracTickets for help on using tickets.