Opened 7 years ago

Closed 7 years ago

#2490 closed task (complete)

Option definitions: multiple data formats to be accepted to create option instance.

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

Currently the OptionDefinition? object returns the instance of the factory function that can be used to create option. This factory function takes OptionBuffer? as an argument. We need other ''data formats'' to be accepted too:

  • Comma separated values: to allow user put values for particular option values separated with comma sign which is more convenient when defining option values in the config manager.
  • iterators: when packet is received from the client it is stored in the buffer that comprises many options. It is required to supply chunk of this buffer, which represents the particular option, to an OptionDefinition? object. This chunk of buffer is specified with two iterators (specifying begin and end of the chunk).

Subtickets

Change History (8)

comment:1 Changed 7 years ago by marcin

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

comment:2 Changed 7 years ago by marcin

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

OptionDefinition? now exposes three optionFactory methods that can be used to create DHCP option instances. The option values can be specified with:

  • two iterators pointing to the begining and end of buffer holding data,
  • buffer that holds data
  • vector of strings, where each string points to the value of a particular option field.

Please review.

comment:3 Changed 7 years ago by stephen

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

comment:4 Changed 7 years ago by stephen

  • Status changed from accepted to reviewing

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

  • Owner changed from stephen to marcin

Reviewed commit c84d8a4bcf3058d49c88962744102cb3efa7aa2c

src/lib/dhcp/libdhcp++.cc
unpackOptions6(): suggest the comment about removing the "else if" in future be an "@todo".

initStdOptionDefs6(): Suggestion - unless the order matters, I'd suggest that the elements of the "params" array be ordered alphabetically by option name. (You may want to see if aligning the elements in columns makes it more readable.)

initStdOptionDefs6(): You say a validation error is a programming error and should never happen, so wouldn't throwing an exception be better? That way the caller is guaranteed that the return value is valid.

src/lib/dhcp/option_data_types.h
I'm not certain that setting "len" to the size of IOAddress or string is useful. As far as I can see, "len" is only used to determine what function to use when writing data to a buffer. It is unlikely - but possible - that some implementations of string have a size of 4 bytes. I would suggest setting their length to 0 (on the grounds that if data in a string has to be written, the amount of data won't be sizeof(string) - similarly for IOAddress) or choosing which function to use on the basis of OptionDataTypeTraits<T>::type

src/lib/dhcp/option_definition.cc
writeToBuffer(): "case OPT_UINT8_TYPE" clause - the cast should be to uint8_t, not int8_t.

Address/String types: nice use of copy_backward to avoid the need to know how much the buffer had been extended by - I've not come across that STL function before.

OPT_STRING_TYPE: suggest value.size() is used instead of valid.length(), to be consistent with other uses.

optionFactory: just a comment that the "try" block is a lot of solid lines of code. I would suggest introducing a blank line before each "else if" statement in the main "if" block and seeing if it is easier to read.

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

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit c84d8a4bcf3058d49c88962744102cb3efa7aa2c

src/lib/dhcp/libdhcp++.cc
unpackOptions6(): suggest the comment about removing the "else if" in future be an "@todo".

Added!

initStdOptionDefs6(): Suggestion - unless the order matters, I'd suggest that the elements of the "params" array be ordered alphabetically by option name. (You may want to see if aligning the elements in columns makes it more readable.)

They are sorted by their codes. This is intentional because it helps me to confront for which options definitions are already implemented, for which they aren't. I am using the DHCPv6 option list on iana.org as a reference. On this site they list options by codes.

initStdOptionDefs6(): You say a validation error is a programming error and should never happen, so wouldn't throwing an exception be better? That way the caller is guaranteed that the return value is valid.

Ok. Maybe throwing exception has more sense. I now throw exception.

src/lib/dhcp/option_data_types.h
I'm not certain that setting "len" to the size of IOAddress or string is useful. As far as I can see, "len" is only used to determine what function to use when writing data to a buffer. It is unlikely - but possible - that some implementations of string have a size of 4 bytes. I would suggest setting their length to 0 (on the grounds that if data in a string has to be written, the amount of data won't be sizeof(string) - similarly for IOAddress) or choosing which function to use on the basis of OptionDataTypeTraits<T>::type

Set to 0.

src/lib/dhcp/option_definition.cc
writeToBuffer(): "case OPT_UINT8_TYPE" clause - the cast should be to uint8_t, not int8_t.

Good catch. Fixed.

Address/String types: nice use of copy_backward to avoid the need to know how much the buffer had been extended by - I've not come across that STL function before.

Actually, I have realized that STL has this function quite recently.

OPT_STRING_TYPE: suggest value.size() is used instead of valid.length(), to be consistent with other uses.

Changed.

optionFactory: just a comment that the "try" block is a lot of solid lines of code. I would suggest introducing a blank line before each "else if" statement in the main "if" block and seeing if it is easier to read.

Blank lines added.

Here is a Changelog entry I am proposing:

509.	[func]		marcin
	DHCPv6 option instances can be created using a collection of strings.
	Each string represents a value of a particular data field within
	an option. The data field values, given as strings, are validated
	against the actual types of option fields specified in options
	definitions.
	(Trac #2490, git c7defffb89bd0f3fdd7ad2437c78950bcb86ad37)

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Here is a Changelog entry I am proposing:

Last two words: s/options defintions/the option definitions/

All OK, please merge.

comment:8 Changed 7 years ago by marcin

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

Merged to master (commit 56cfd6612fcaeae9acec4a94e1e5f1a88142c44d).

Note: See TracTickets for help on using tickets.