Opened 7 years ago

Closed 7 years ago

#2187 closed enhancement (fixed)

Remove "cout" statements from libdhcp++

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

Although BIND 10 DHCP will ultimately use the BIND 10 logging framework, to allow use of libdhcp++ outside BIND 10 DHCP, errors etc. will be signalled using exceptions. (This is similar to libdns++.)

This tasks removes the "cout" statements from libdhcp++. Where it is particularly important that something be logged, a "TODO" comment will be added - these will be addressed (and the comments removed) when #1545 is done.

Note: #1545 is now dependent on this ticket.

Subtickets

Change History (4)

comment:1 Changed 7 years ago by marcin

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

comment:2 Changed 7 years ago by marcin

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

The code is now ready for review:

  • I removed all informational cout statements from lib/dhcp
  • I throw exception when: detectIfaces, receiveX or send fails (instead of printing error)
  • I catch exceptions produced by receiveX and send in DHCP servers and log them
  • I also improved some doxygen comments to indicate exceptions being thrown
  • I did slight changes to perfdhcp as it uses IfaceMgr?.

Note: I don't throw exceptions when things like pack() fail because we spoke with Stephen
that we would change those functions to void and throw exceptions with another ticket.
I simply added @todo statements there.

Please review.

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

  • Owner changed from UnAssigned to marcin

Note: I don't throw exceptions when things like pack() fail because we spoke with Stephen that we would change those functions to void and throw exceptions with another ticket. I simply added @todo statements there.

The ticket is #2264


Reviewed commit 02dd6ed0dd458e7627e06bcab6a5168354e929d6

I've made (and pushed) minor changes to some text in the following files:

  • src/bin/dhcp4/dhcp4_messages.mes
  • src/bin/dhcp6/dhcp6_messages.mes
  • src/lib/dhcp/iface_mgr.cc

Other comments:

src/lib/dhcp/tests/iface_mgr_unittest.cc
Test socketsFromRemoteAddress: suggest moving the call to ifaceMgr->closeSockets() to the end of the function (after the #endif for OS_LINUX). That way, if running the test on Linux, socket3 will be closed on exit as well.

tests/tools/perfdhcp/test_control.cc
receivePackets(): messages written to cout should probably be written to cerr (as they are error messages).

Unless you alter something else, I don't need to see the corrections. Please merge after you've made (and tested) then.

comment:4 in reply to: ↑ 3 Changed 7 years ago by marcin

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

Replying to stephen:

Note: I don't throw exceptions when things like pack() fail because we spoke with Stephen that we would change those functions to void and throw exceptions with another ticket. I simply added @todo statements there.

The ticket is #2264


Reviewed commit 02dd6ed0dd458e7627e06bcab6a5168354e929d6

I've made (and pushed) minor changes to some text in the following files:

  • src/bin/dhcp4/dhcp4_messages.mes
  • src/bin/dhcp6/dhcp6_messages.mes
  • src/lib/dhcp/iface_mgr.cc

Thanks for your changes.

Other comments:

src/lib/dhcp/tests/iface_mgr_unittest.cc
Test socketsFromRemoteAddress: suggest moving the call to ifaceMgr->closeSockets() to the end of the function (after the #endif for OS_LINUX). That way, if running the test on Linux, socket3 will be closed on exit as well.

I can't move closeSockets() because it guarantees that when part of the test enclosed in #if OS_LINUX starts all sockets are closed. This is required because one of the ports is reused in this part. Attempt to close another socket using the same port would result in failure.

In the same time there is no need to invoke closeSockets() at the end of the function because it is invoked anyway by the IfaceMgr?'s virtual destructor.

I added some comments on this in the code.

tests/tools/perfdhcp/test_control.cc
receivePackets(): messages written to cout should probably be written to cerr (as they are error messages).

Ok, replaced cout with cerr.

Unless you alter something else, I don't need to see the corrections. Please merge after you've made (and tested) then.

Code is merged. Thanks for review.

Note: See TracTickets for help on using tickets.