Opened 9 years ago

Closed 8 years ago

#992 closed enhancement (fixed)

DHCP IPv4 server as a dummy BIND 10 component

Reported by: shane Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20111221
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.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Set up a DHCP IPv4 server as a dummy BIND 10 component.

Note that to do any real work here we need to figure out how to handle the lower-level networking issues. For IPv4 on the server this means being able to send a packet to a client without an address. That task is described in ticket #991.

Subtickets

Change History (13)

comment:1 Changed 9 years ago by shane

  • Milestone changed from New Tasks to DHCP 2011

comment:2 Changed 8 years ago by stephen

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

comment:3 Changed 8 years ago by tomek

  • Component changed from Unclassified to dhcp4

comment:4 Changed 8 years ago by tomek

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

comment:5 Changed 8 years ago by tomek

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

Skeleton code for DHCPv4 server daemon created. It does not do anything useful yet.

This code requires ticket 1238.

There are several (very small) pieces of code that will be uncommented once #1350, #1239 and #1240 are implemented.

There are several skeleton functions. It does not make sense to write dummy tests for them before they get a real implementation.

Code ready for review.

comment:6 Changed 8 years ago by stephen

There are several skeleton functions. It does not make sense to write dummy tests for them before they get a real implementation.

Actually it does. Test-driven development requires that the tests be written first; if nothing else, it checks that the tests do fail if the code does nothing.

comment:7 Changed 8 years ago by tomek

Implemented trivial tests for Dhcpv4srv::process*. They will be expanded once we reach stage of actually implementing those methods. processDiscover() and processRequest() are like to be implemented first.

Please review.

comment:8 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to tomek

Reviewed commit d8cd199a66645341270081a7f409c557e596099b

src/bin/dhcp4/dhcp4.spec
Just a comment that we will eventually need a list of interfaces. However it is fine for now.

src/bin/dhcp4/dhcp4_srv.h
Dhcp4Srv contructor: port argument needs Doxygen entry in the header.

processXxx(): I feel that the arguments should be passed by reference to avoid the overhead of a reference count increase during the class. (Although I know Jinmei disagrees with me on this one.)

shutdown member variable should be shutdown_

src/bin/dhcp4/dhcp4_srv.cc
run(): In the longer term we need to address how to shutdown the server if it is in the receive4() call when the shutdown request is received.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
The comment should be '"naked" DHCP4 server', not '"naked" Interface Manager'

In NakedDhcpv4Srv(), if you don't change the function signatures you don't need to duplicate base class methods. (In this case, the derived class receives the parameters by value and passes them to the base class by reference; if it were to received the parameters by reference the methods would be no-op and could be omitted.)

"basic" test: there is no need to check is srv is non-null before attempting the deletion.

processXxx tests: Do we want to do an ASSERT_NO_THROW around the "delete src"?

src/bin/dhcp4/tests/dhcp4_unittests.cc
"return result;" should be "return (result);" according to the coding guidelines.

src/lib/dhcp/iface_mgr.*
I didn't review these files or tests as they receintly appeared in another ticket that I reviewed and I understand there is a lot of cross-over between tickets at the moment.

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

Replying to stephen:

Reviewed commit d8cd199a66645341270081a7f409c557e596099b

src/bin/dhcp4/dhcp4.spec
Just a comment that we will eventually need a list of interfaces. However it is fine for now.

src/bin/dhcp4/dhcp4_srv.h
Dhcp4Srv contructor: port argument needs Doxygen entry in the header.

Added.

processXxx(): I feel that the arguments should be passed by reference to avoid the overhead of a reference count increase during the class. (Although I know Jinmei disagrees with me on this one.)

Added references. Note that this should not be const reference as there is a possibility that packets will be modified, e.g. if incoming packet does not have PRL, it would be useful to add empty PRL for easier processing.

shutdown member variable should be shutdown_

Done.

src/bin/dhcp4/dhcp4_srv.cc
run(): In the longer term we need to address how to shutdown the server if it is in the receive4() call when the shutdown request is received.

Yes. receive4() and receive6() also will take a timeout parameter that specifies how long reception attempt should take. The most obvious use for this is lease expiration. Exact way of handling shutdown (or any other control sequence) is to be determined. Perhaps receive4() and receive6() will be called in a separate thread? That requires architectural discussion.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
The comment should be '"naked" DHCP4 server', not '"naked" Interface Manager'

In NakedDhcpv4Srv(), if you don't change the function signatures you don't need to duplicate base class methods. (In this case, the derived class receives the parameters by value and passes them to the base class by reference; if it were to received the parameters by reference the methods would be no-op and could be omitted.)

I have to. Although signatures are the same, access specifiers are different. Naked server has them public, so they can be used in tests.

"basic" test: there is no need to check is srv is non-null before attempting the deletion.

Fixed.

processXxx tests: Do we want to do an ASSERT_NO_THROW around the "delete src"?

Although we did this in many tests, I think it is redundant. Test will fail anyway if exception is thrown in code that was not explicitly expected (EXPECT_THROW or ASSERT_THROW). As this is the last statement in every test, it does not make much sense to add it. I we should do something opposite eventually, i.e. remove EXPECT_NO_THROW() in many tests.

src/bin/dhcp4/tests/dhcp4_unittests.cc
"return result;" should be "return (result);" according to the coding guidelines.

Done.

src/lib/dhcp/iface_mgr.*
I didn't review these files or tests as they receintly appeared in another ticket that I reviewed and I understand there is a lot of cross-over between tickets at the moment.

Yes, that is true. This code went through significant changes recently. Perhaps after end of Dec. release, we could deal with some tickets that address rewriting Pkt6 to use vector<uint8_t> and then re-review updated IfaceMgr? again?

Added ChangeLog? entry.

Please review.

Last edited 8 years ago by tomek (previous) (diff)

comment:11 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

comment:12 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

All looks OK, please merge.

Yes, that is true. This code went through significant changes recently. Perhaps after end of Dec. release, we could deal with some tickets that address rewriting Pkt6 to use vector<uint8_t> and then re-review updated IfaceMgr again?

Yes.

comment:13 Changed 8 years ago by tomek

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

Changes merged and pushed to master.

Note: See TracTickets for help on using tickets.