Opened 8 years ago

Closed 7 years ago

#1960 closed enhancement (fixed)

perfdhcp integration

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

Integrate the converted perfdhcp components, update build script and tidy up.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by stephen

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

comment:2 Changed 7 years ago by marcin

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

comment:3 Changed 7 years ago by marcin

A list of proposed changes that had come up during review of #1959 and were postponed to #1960:

  • getValue(), getName() and similar - for simple methods no repetition of \brief descriptions are needed, the brief description is sufficient.
  • Consider renaming CommandOptions? to something else that lacks word "Options" as it has special meaning in DHCP.
  • Check if multicast flag is needed for DHCPv6 multicast remote addresses.
  • Consider preparing common ancestor for StatsMgr? to have single pointer to StatsMgr4 and StatsMgr6.
  • openSocket(): Use port 68 for DHCPv4.
  • sendRequest4(): What happens if there is on server-id in the ADVERTISE message and the exception is thrown? Perhaps we should let the test continue, but increase some form of "RFC3315 violations" counter. The run will ultimately fail, but it is often interesting not abort after first fail, but let the server continue its work.
  • Consider moving CommandOptionsHelper? to common place.

comment:4 Changed 7 years ago by marcin

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

With this ticket all listed issues have been resolved except:

  • Moving CommandOptionsHelper? to common place. Postponed to another ticket.
  • Rename CommandOptions? to something else. Other tools already use CommandOptions? name (e.g. tests/tools/badpacket). It is good to have common names across the code. Please suggest during a review if this should be renamed anyway.

Additional changes:

  • the old perfdhcp code has been removed.
  • diagnostics flag 'r' was removed from program usage because it is not implemented in the code (the randomization details aka. number of clients is always printed).

Please review.

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to marcin

Reviewed commit 5da8f0ce275404a225759f9673daffa4d9da386a

tests/tools/perfdhcp/command_options.cc
initialize(): Why not have the list of switches in the "getopt" argument in alphabetical order?

initialize(): the list of options in the various "case" labels is in alphabetical order apart from "v". Suggest moving it to the appropriate place.

initialize(): As "255.255.255.255" and "FF02::1:2" are used more than once, why not define symbols for them? (In fact, as the program links against libdhcp++, it is permissible for it to use symbols from there. dhcp6.h defines the symbols for the DHCP6 broadcast addresses, and dhcp4.h could be extended to define something like DHCP_IPV4_BROADCAST_ADDRESS).

initialize(): At the block of code with the comment "Handle the local '-l' address/interface", the variable broadcast_ is set to 1; as broadcast_ is a bool, it should be set 'true'.

initClientsNum(): isc_throw is passed 'errmsg.c_str()' in the first invocation and 'errmsg' in the second.

initClientsNum(): the use of two try/catch blocks is not needed: a single one is sufficient:

const std::string errmsg = "...";
try {
    long long client_num = boost::lexical_cast<long long>(optarg);
    check(client_num < 0, errmsg);
    clients_num_ = boost::lexical_cast<uint32_t>(optarg);
} catch (boost::lexical_cast&) {
    isc_throw(isc::InvalidParameter, errmsg);
}

(If check() fails, it throws an isc::InvalidParameter exception, which will not be caught by the "catch" clause.)

decodeMac(): the error message states that MAC components should be separated by double-colons: is this correct?

tests/tools/perfdhcp/main.cc
If an error message is output, shouldn't it be written to cerr, not cout?

tests/tools/perfdhcp/stats_mgr.h
CustomCounter::operator+=(): Could use counter_ += val construct.

tests/tools/perfdhcp/test_control.cc
receivePackets(): return value should be enclosed in parentheses.

run() is a bit long: can it be split up at all (e.g. move the "preload server" loop into a separate method, put the printing of diagnostic messages into a separate method etc.)?

run(): when calling runWrapped(do_stop), why the intermediate variable do_stop? Why not just call runWrapped() with a literal value of "true"? Also, the reason for this call needs a comkment.

sendDiscover4(): add a comment before the testDiags('T')? Not clear what is happening here.

sendRequest6(): A comment would help in at the head of the first "if" in this method. Why are you testing the result of the CommandOptions::instance()::isUseFirst() method?

tests/tools/perfdhcp/test_control.h
The header comments for the TestControl class needs to be extended. For example, TestControl appears to start subprocesses - there is no indication in the header that this is done. Is a subprocess started in all cases, or just some? What does it do? etc.

I presume that the offset variables are offsets in the DHCP packet. If so, this should be stated (the comments only mention "offsets").

Typo in the name of a class: "Sequencial" should be "Sequential".

Typo in SequentialGenerator header - "maximym".

SequentialGenerator: strictly speaking, range_ is not the maximum number generated, it is one more than the maximum number: (num_ + 1) % range_ returns values between 0 and range_ - 1 inclusive.

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

Replying to stephen:

Reviewed commit 5da8f0ce275404a225759f9673daffa4d9da386a

tests/tools/perfdhcp/command_options.cc
initialize(): Why not have the list of switches in the "getopt" argument in alphabetical order?

initialize(): the list of options in the various "case" labels is in alphabetical order apart from "v". Suggest moving it to the appropriate place.

Moved.

initialize(): As "255.255.255.255" and "FF02::1:2" are used more than once, why not define symbols for them? (In fact, as the program links against libdhcp++, it is permissible for it to use symbols from there. dhcp6.h defines the symbols for the DHCP6 broadcast addresses, and dhcp4.h could be extended to define something like DHCP_IPV4_BROADCAST_ADDRESS).

Added define as suggested.

initialize(): At the block of code with the comment "Handle the local '-l' address/interface", the variable broadcast_ is set to 1; as broadcast_ is a bool, it should be set 'true'.

Fixed.

initClientsNum(): isc_throw is passed 'errmsg.c_str()' in the first invocation and 'errmsg' in the second.

Fixed.

initClientsNum(): the use of two try/catch blocks is not needed: a single one is sufficient:

const std::string errmsg = "...";
try {
    long long client_num = boost::lexical_cast<long long>(optarg);
    check(client_num < 0, errmsg);
    clients_num_ = boost::lexical_cast<uint32_t>(optarg);
} catch (boost::lexical_cast&) {
    isc_throw(isc::InvalidParameter, errmsg);
}

(If check() fails, it throws an isc::InvalidParameter exception, which will not be caught by the "catch" clause.)

I got rid of second clause as suggested.

decodeMac(): the error message states that MAC components should be separated by double-colons: is this correct?

It can be even more colons. I extended error message to point out that single colon is fine too.

tests/tools/perfdhcp/main.cc
If an error message is output, shouldn't it be written to cerr, not cout?

Fixed.

tests/tools/perfdhcp/stats_mgr.h
CustomCounter::operator+=(): Could use counter_ += val construct.

Fixed.

tests/tools/perfdhcp/test_control.cc
receivePackets(): return value should be enclosed in parentheses.

Fixed.

run() is a bit long: can it be split up at all (e.g. move the "preload server" loop into a separate method, put the printing of diagnostic messages into a separate method etc.)?

I made it shorter. For example I created new function sendPackets() that is used both to preload the server and send packets when not preloading.

run(): when calling runWrapped(do_stop), why the intermediate variable do_stop? Why not just call runWrapped() with a literal value of "true"? Also, the reason for this call needs a comkment.

I added some more comments about wrapped commands.

sendDiscover4(): add a comment before the testDiags('T')? Not clear what is happening here.

Since testDiags('T') repeats in every sendX() function I created the function which does testDiags('T') and saves the first packet. Its description tells what is going on there.

sendRequest6(): A comment would help in at the head of the first "if" in this method. Why are you testing the result of the CommandOptions::instance()::isUseFirst() method?

Comment added.

tests/tools/perfdhcp/test_control.h
The header comments for the TestControl class needs to be extended. For example, TestControl appears to start subprocesses - there is no indication in the header that this is done. Is a subprocess started in all cases, or just some? What does it do? etc.

I extended the class description.

I presume that the offset variables are offsets in the DHCP packet. If so, this should be stated (the comments only mention "offsets").

Added. I hope that everywhere it was missing.

Typo in the name of a class: "Sequencial" should be "Sequential".

I renamed the class.

Typo in SequentialGenerator header - "maximym".

Fixed.

SequentialGenerator: strictly speaking, range_ is not the maximum number generated, it is one more than the maximum number: (num_ + 1) % range_ returns values between 0 and range_ - 1 inclusive.

I changed the description.

Apart from changes suggested in the review I also added EXPECT_NO_THROW to each call to process() function in command_options_unittest where I don't expect it to throw.

I fixed some doxygen issues.

I also corrected the configure.ac as there was some redundant code there (for old perfdhcp).

comment:8 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

As suggested in #2230 the packet templates should allow spaces and I added it here.

comment:9 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 2538cdc2251cc27c38193680af90f2e7e94ffb57

tests/tools/perfdhcp/test_control.h
I've corrected some typos etc. in comments and pushed the modified file.

tests/tools/perfdhcp/command_options.cc
Do not appear to have sorted the list of switches in the getopt() call in alphabetical order. However, don't bother to change it now - it works, I just thought that the switches looked more logical when arranged alphabetically.

tests/tools/perfdhcp/tests/command_options_unittest.cc
Could encapsulate calls to process()

It can be even more colons. I extended error message to point out that single colon is fine too.

OK - I just thought it was usual to separate the numbers in the MAC address with single colons, which was why I asked the question.

The unit tests failed:

command_options_unittest.cc:151: Failure
Value of: process("perfdhcp -6 -l ethx -h all")
  Actual: false
Expected: true
VERSION: 20120817
command_options_unittest.cc:152: Failure
Value of: process("perfdhcp -l ethx -v all")
  Actual: false
Expected: true
Running: perfdhcp -l ethx
[  FAILED  ] CommandOptionsTest.HelpVersion (13 ms)

This was due to a problem in the wrapper function CommandOptionsTest::process() is declared bool but did not contain a "return" statement. Surprisingly, it compiled without complaint on my Linux system. I've fixed the test (returned the status from CommandOptionsHelper::process()) and it now passes. The updated has been pushed.


If you're happy with the changes I've made, you can merge.

comment:10 Changed 7 years ago by tomek

I just reviewed three last commits (d98f7060780fc89f05c1610d3e88403969dfc819, 85c7fb2e4627e84c2d17c59432509a221034e6ea and d570aa48262648cfccdf96e01b1c7c66a9d0839d). They are ok.

comment:11 Changed 7 years ago by marcin

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

Thanks for your reviews and changes. Code has been merged.

Note: See TracTickets for help on using tickets.