Opened 8 years ago

Closed 8 years ago

#1239 closed task (fixed)

Receiving data over V4

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20111230
Component: dhcp4 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

Handling the reception of incoming packets over IPv4.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2011 to Sprint-DHCP-20111109

comment:2 Changed 8 years ago by tomek

  • Component changed from dhcp to dhcp4

comment:3 Changed 8 years ago by tomek

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

comment:4 Changed 8 years ago by tomek

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

Code is implemented and in ready for review.

Due to severe backlog in reviewing older tickets, trac1239 will be somewhat difficult to review. It includes merged code from branches #1238 (IPv4 support in IfaceMgr?) and #992 (dummy dhcp4 component). It used to require code from #1237 (Linux interface detection), but the code was modified to work without interface detection.

I'm sorry for the unfortunate state of this branch, but I cannot review my own code.

Note: The original code is available on experiments/dhcp4 branch. I'm using this branch for normal development and then merge changes to separate ticket branches. It is not perfect solution as merging is required one way or the other. However, having a single, unified working code may come very handy in pessimistic scenario.

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

  • Owner changed from UnAssigned to tomek

Reviewing changes from 1ee6d8e7f892d929321b32e5577eb3a3be65a15e (Merge of 1238) to 5be5e6a639a6a1c74761cae55a97f1fa46de5c6d

src/bin/dhcp4/dhcp4_srv.cc
No need to change this, as it is in a temporary debug output statement, but should use the static_cast<int>() construct to convert to an int. (Although as Pkt4::getType() returns a uint8_8, is there a need to convert? Can't the ostream "<<" operator output uint8_t data?)

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Dhcpv4SrvTest() constructor - unlink() should be indented by one more space.

Dhcpv4SrvTest() constructor - "|" and ">" operators should have spaces round them.

Dhcpv4SrvTest() constructor - should the code be reading "127.0.0.1" instead of "::1" for an IPV4 test?

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Dhcpv6SrvTest() constructor - "|" and ">" operators should have spaces round them.

There appears to be some commonality between the v4 and v6 tests that we should perhaps explore at some later time.

src/lib/dhcp/iface_mgr.{cc,h}
Not reviewed - these appear to be earlier version of code reviewed in another ticket.

src/lib/dhcp/option.cc
getUint16() and getUint32(): Rather than do the bit manipulation here, there are functions available in src/lib/util/io_utilities.h. Using these may make the code clearer.

src/lib/dhcp/option.h
getUintXX() methods require Doxygen headers.

src/lib/dhcp/pkt4.cc
pack(): Need spaces around ">".

src/lib/dns/benchmarks/Makefile
For some reason, this file appears to have been added to git in this branch - it should be removed.

comment:6 in reply to: ↑ 5 Changed 8 years ago by tomek

Replying to stephen:

Reviewing changes from 1ee6d8e7f892d929321b32e5577eb3a3be65a15e (Merge of 1238) to 5be5e6a639a6a1c74761cae55a97f1fa46de5c6d

src/bin/dhcp4/dhcp4_srv.cc
No need to change this, as it is in a temporary debug output statement, but should use the static_cast<int>() construct to convert to an int. (Although as Pkt4::getType() returns a uint8_8, is there a need to convert? Can't the ostream "<<" operator output uint8_t data?)

Yes, it can, but uint8_t is really unsigned char, so this prints out mostly unreadable characters with ASCII code 1 to 9. I want them printed as integer values.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Dhcpv4SrvTest() constructor - unlink() should be indented by one more space.

Fixed.

Dhcpv4SrvTest() constructor - "|" and ">" operators should have spaces round them.

Done.

Dhcpv4SrvTest() constructor - should the code be reading "127.0.0.1" instead of "::1" for an IPV4 test?

My initial idea was to keep it as IPv6 addresses, so openSockets4() wouldn't open any sockets (to avoid problems with running tests as non-root). However, I now modified the code to follow dhcp6. There is constructor parameter that specifies port on which sockets should be opened. Tests now open sockets (on port 10067) and pass, when run as regular user.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Dhcpv6SrvTest() constructor - "|" and ">" operators should have spaces round them.

Done.

There appears to be some commonality between the v4 and v6 tests that we should perhaps explore at some later time.

My gut feeling is that the code will diverge. There will be new parameters that are IP-family specific.

src/lib/dhcp/iface_mgr.{cc,h}
Not reviewed - these appear to be earlier version of code reviewed in another ticket.

src/lib/dhcp/option.cc
getUint16() and getUint32(): Rather than do the bit manipulation here, there are functions available in src/lib/util/io_utilities.h. Using these may make the code clearer.

Now that you mentioned those methods, also tests for getUintX() would be useful. I think I'm getting addicted to TDD. :)

Implemented OptionTest?.getUintX. Once the test code was working, I refactored getUintX() to use code from io_utilities.h. Note that Jinmei was against using io_utilities code and said that the code should go away.

src/lib/dhcp/option.h
getUintXX() methods require Doxygen headers.

Documented.

src/lib/dhcp/pkt4.cc
pack(): Need spaces around ">".

I assume you meant "unpack()". Fixed that. There is no ">" in Pkt4::pack().

src/lib/dns/benchmarks/Makefile
For some reason, this file appears to have been added to git in this branch - it should be removed.

For some reason I must have added this Makefile. I should be more careful. Removed.

Changes checked in. Please review.

comment:7 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

comment:8 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

src/lib/dhcp/option.h
The comments

// This method will throw OutOfRange if option has 0 length
// This method will throw OutOfRange if option has 2 length
// This method will throw OutOfRange if option has 4 length

are better expressed as

// @exception OutOfRange Thrown if the option has a length of zero
// @exception OutOfRange Thrown if the option has a length of two
// @exception OutOfRange Thrown if the option has a length of four

Also (in the header for getUint32), there is no return type of uint14_t :-)

Unit Tests
These failed with the messages:

IfaceMgr initialization.
Interface detection is not implemented yet. Reading interfaces.txt file instead.
Please use format: interface-name link-local-address
Failed to read interfaces.txt file.
Interface detection failed.
IfaceMgr creation failed:std::exception
terminate called after throwing an instance of 'std::exception'
  what():  std::exception
/bin/bash: line 5: 32161 Aborted                 ${dir}$tst

Should there be an "interfaces.txt" file in the repository.

comment:9 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Removed reference to uint14_t as it seems to be not supported :)
Reworded comments regarding exceptions (now using @exception keyword).
Merged fix from master, also cleaned up two tests failing on Mac OS.

Please review.

comment:10 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 84ef9da129de44dfa74933f83db60bdaf2dce51e

I've made and pushed two small changes:
a) removed spurious spaces in a statement in src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
b) Added liblog to libraries to link against in src/bin/dhcp6/tests/Makefile.am to get round a link error.

If you are happy with these changes, it is OK to merge.

comment:11 Changed 8 years ago by tomek

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

Merged.

Note: See TracTickets for help on using tickets.