Opened 7 years ago

Closed 6 years ago

#2264 closed enhancement (fixed)

Pkt4::pack() and Pkt6::pack() should throw an exception on failure

Reported by: stephen Owned by: tmark
Priority: medium Milestone: Sprint-DHCP-20130731
Component: dhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The pack() methods of Pkt4 and Pkt6 currently return a bool value, with false indicating a failure. A failure can also be signalled by an exception being thrown. This ticket removes the inconsistency: the functions should be declared void and packing errors indicated by the throwing of an exception. The code where they are used (in the DHCP4/6 server) must be modified to handle the change.

For more details, see this comment on ticket #1545.

Subtickets

Attachments (1)

patch.2264 (7.5 KB) - added by dclink 6 years ago.
Update of both dhcp4 and dhcp6 server class and ptk4 and ptk6 ones as well.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130328

comment:2 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20130509 to DHCP Outstanding Tasks

Changed 6 years ago by dclink

Update of both dhcp4 and dhcp6 server class and ptk4 and ptk6 ones as well.

comment:3 Changed 6 years ago by dclink

About ptk6 case, I made pack<something>void return as well. I think it is better like that.

comment:4 Changed 6 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130731

comment:5 Changed 6 years ago by tomek

Thanks for your patch. We'll take a look as soon as possible.

Ok, we have an external patch being contributed. Great! I've added it to our current sprint. Let's take a look at this sooner rather than later.

comment:6 Changed 6 years ago by tmark

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Changes made under branch, "trac2264", git commit: f395f315a51a23ac058456ec15580231f145bd75

The patch as received needed some minor corrections.

  1. The unit test code did not compile:
    :
    pkt6_unittest.cc: In member function 'virtual void<unnamed>::Pkt6Test_packUnpack
    _Test::TestBody()':
    pkt6_unittest.cc:224: error: invalid use of void expression
    pkt6_unittest.cc: In member function 'virtual void<unnamed>::Pkt6Test_relayPack_
    Test::TestBody()':
    pkt6_unittest.cc:518: error: invalid use of void expression
    *** Error code 1
    :
    

The code was still using EXPECT_TRUE macro. These were replaced with EXPECT_NO_THROW.
Once replace these two tests failed to execute:

[ RUN      ] Pkt6Test.packUnpack
pkt6_unittest.cc:225: Failure
Expected: parent->pack() doesn't throw an exception.
  Actual: it throws.
[  FAILED  ] Pkt6Test.packUnpack (0 ms)
[ RUN      ] Pkt6Test.addGetDelOptions
[       OK ] Pkt6Test.addGetDelOptions (0 ms)
[ RUN      ] Pkt6Test.Timestamp
[       OK ] Pkt6Test.Timestamp (0 ms)
[ RUN      ] Pkt6Test.getName
[       OK ] Pkt6Test.getName (0 ms)
[ RUN      ] Pkt6Test.relayUnpack
[       OK ] Pkt6Test.relayUnpack (1 ms)
[ RUN      ] Pkt6Test.relayPack
pkt6_unittest.cc:520: Failure
Expected: parent->pack() doesn't throw an exception.
  Actual: it throws.
[  FAILED  ] Pkt6Test.relayPack (0 ms)
[ RUN      ] Pkt6Test.getAnyRelayOption
[       OK ] Pkt6Test.getAnyRelayOption (0 ms)
[----------] 9 tests from Pkt6Test (4 ms total)

Closer inspection revealed the Packet6::pack was missing "break" statements inside the switch.
Once corrected, all unit tests passed.

Beyond that there were some minor line-length and indentation issues (see coding guidelines). In all fairness to the patch author, the surrounding code isn't exactly compliant either ;).

comment:7 Changed 6 years ago by dclink

Apologies, did not think to change unit tests ... Will correct all of it in some hours.

Last edited 6 years ago by dclink (previous) (diff)

comment:8 Changed 6 years ago by tmark

DCLINK - The tests have been corrected already. You don't need to do anything further.

But for future reference the unit tests must always be verified with every ticket. Sometimes they need to be modified sometimes not dependent on the scope and nature of the change. Issue
the command "make check" to execute the unit tests.

comment:9 Changed 6 years ago by dclink

tmark No problems I will remember.

comment:10 Changed 6 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:11 follow-up: Changed 6 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit f395f315a51a23ac058456ec15580231f145bd75

I think change is sufficiently small enough not to need a ChangeLog entry.

src/bin/dhcp4/dhcp4_srv.cc
The change has removed the DHCP4_PACK_FAIL error; this should be removed from the message file as well.

src/bin/dhcp6/dhcp6_srv.cc
The change has removed the DHCP6_PACK_FAIL error; this should be removed from the message file as well.

src/lib/dhcp/pkt6.cc
src/lib/dhcp/pkt6.{cc,h}
packUDP(): Method header needs to be altered. The method has been implemented and throws an InvalidOperation exception on an error.

packTCP(): Method header needs to be altered to indicate the exception thrown. Currently the code throws the "Unexpected" exception: I suggest that "NotImplemented" would be a better choice. (If this is changed, the header for pack() needs to be altered.)

comment:12 in reply to: ↑ 11 Changed 6 years ago by tmark

  • Owner changed from tmark to stephen

Replying to stephen:

Reviewed commit f395f315a51a23ac058456ec15580231f145bd75

I think change is sufficiently small enough not to need a ChangeLog entry.

src/bin/dhcp4/dhcp4_srv.cc
The change has removed the DHCP4_PACK_FAIL error; this should be removed from the message file as well.

Message removed.

src/bin/dhcp6/dhcp6_srv.cc
The change has removed the DHCP6_PACK_FAIL error; this should be removed from the message file as well.

Message removed.

src/lib/dhcp/pkt6.cc
src/lib/dhcp/pkt6.{cc,h}
packUDP(): Method header needs to be altered. The method has been implemented and throws an InvalidOperation exception on an error.

Corrected.

packTCP(): Method header needs to be altered to indicate the exception thrown. Currently the code throws the "Unexpected" exception: I suggest that "NotImplemented" would be a better choice. (If this is changed, the header for pack() needs to be altered.)

Method changed to throw NotImplemented. Method commentary changed accordingly.
Changes commited under git 27d057a79de7e769cef2a87e59e08807491bf44e

comment:13 Changed 6 years ago by stephen

  • Owner changed from stephen to tmark

All OK, please merge.

comment:14 Changed 6 years ago by tmark

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

Changes merged: commit 9388cae1ccc1d3e5c6934c47c857a61ab17cf52e

Note: See TracTickets for help on using tickets.