Opened 8 years ago

Closed 7 years ago

#1957 closed enhancement (fixed)

Implement perfdhcp Interface Manager

Reported by: stephen Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20120703
Component: perfdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 24 Add Hours to Ticket: 2
Total Hours: 29 Internal?: no

Description

Extract the perfdhcp interface handling into a separate class.

Subtickets

Change History (8)

comment:1 Changed 7 years ago by stephen

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

comment:2 Changed 7 years ago by tomek

It is strongly suggested to take a look at isc::dhcp::IfaceMgr? class. Perhaps it could be reused in perfdhcp?

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

  • Add Hours to Ticket changed from 0 to 25
  • Estimated Difficulty changed from 0 to 24
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 25

Since dhcp::IfaceMgr? already has lots of useful code, I simply extended it with the following features required by perfdhcp:

  • open socket for specified interface (interface name is specified from command line)
  • open socket for specified local addres (local address can be specified from command line instead of interface name)
  • open socket for specified remote (server) address (server address can be specified from command line)

Also, I added corresponding unit tests for new methods.

Please review the code on trac1957.

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to marcin

Reviewed commit 0e1996af4d06ab528f76e13a3ce7c4c29b12d046.

Some minor changes to spacing around operators ands header comments have been made and pushed.

src/lib/dhcp/iface_mgr.{h,cc}
openSocketFromAddress()/openSocketFromIface(): the description says that the method will return the socket descriptor if the operation was successful. However, if neither the interface nor address is found, the methods will return a value of zero. Two options:

a) Any failure - including not finding the interface or not finding the address - should result in an exception.

b) If a return value is required in the case of not finding an interface or address, I would suggest a value of -1, as 0 is a valid descriptor number.

(In both cases, the variable "sock" can be declared at the point of first use. In case (b), the return status can be hard-coded - there is no need to use "sock".)

getLocalAddress(): as remote_endpoint is not being shared outside this method, a shared_ptr is not needed - a scoped_ptr eliminates the overhead of reference counting.

src/lib/dhcp/tests/iface_mgr_unittest.cc
As a general rule, I suggest that the pointer to NakedIfaceMgr be encapsulated in a scoped_ptr and the "delete" at the end of the method removed. Although OK now, if the code is subsequenly edited to include an ASSERT_XXX test, a memory leak could ensue.

socketsFromIface: the comment says that the v4 sicket (sic) be bound to the 10548 port, yet the code binds it to 10547.

Suggest that the "magic" port numbers 10547 and 10548 be coded as symbolic constants.

comment:6 Changed 7 years ago by marcin

  • Add Hours to Ticket changed from 25 to 2
  • Owner changed from marcin to stephen
  • Total Hours changed from 25 to 27

Good suggestions.
Code now throws exception if address to bind socket to is not found.
Other suggested changes applied as well.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 66d9922628755b55d297c03e02d5c7fb2614f8ac

All OK, but I made (and pushed) a couple of minor changes to iface_mgr_unittest.cc: the definition of the port numbers is now common to all tests, and I changed buf_size to BUF_SIZE (as it is an external constant).

If you are happy with the changes, please merge.

comment:8 Changed 7 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 27 to 29

Code has been merged to master.

Note: See TracTickets for help on using tickets.