Opened 8 years ago

Closed 7 years ago

#1959 closed enhancement (fixed)

Implement perfdhcp control logic

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

Convert the perfdhcp control login to C++

Subtickets

Change History (25)

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 assigned

comment:3 Changed 7 years ago by marcin

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

The test control logic has been implemented including support for packet templates and dynamic build of packets if template files not specified.

The main test control loop is responsible for exchanges rate control. In order to prevent frequent main loop execution it is recommended to implement nano/miliseconds timeouts for IfaceMgr::receiveX functions. Non-zero timeouts would block on receiveX operations for a time to be waited to initiate new exchange. We should probably open another ticket for this?

The master has been merged to trac1959 during implementation to pull in Statistics Manager from ticket #1958. This may make it a little difficult to browse changes on trac1959 but merge was neccessary. The following files have been created on the course of this ticket and have to be reviewed:

  • test_control.cc
  • test_control.h
  • tests/test_control_unittest.cc
  • templates/discover-example.hex (hex packet dump)
  • templates/request4-example.hex (hex packet dump)
  • templates/solicit6-example.hex (hex packet dump)
  • templates/request6-example.hex (hex packet dump)

Also the other files that have been changed:

  • stats_mgr.h
  • pkt_transform.h
  • perf_pkt6.h (and .cc)
  • perf_pkt4.h (and .cc)

Please review branch trac1959.

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 Changed 7 years ago by tomek

  • Owner changed from stephen to tomek

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

First, let me say that this ticket is huge (the diff is 5400 lines long). In future tickets, it would be reasonable to split the work into separate tickets to make it more manageable.

src/lib/dhcp/iface_mgr.cc
Please remove cout statement in closeSockets(). There's a separate ticket for removing cout in the whole libdhcp library. Let's not add new work to it.

getLocalAddress()
How is the code for broadcast is going to work on a device with multiple interfaces? 255.255.255.255 is a local network broadcast address. If your host is connected to 2 networks, it is valid on both. There should be a way to specify which interface is going to be used. I'm not sure how to do it with boost (and IPv4), but there is a field in6_scope_id that can be used for scope in IPv6. Perhaps something similar is available for IPv4. If it isn't, we should at least mention it in the docs. (od add @todo note somewhere).

openSocket6()
if (addr.toText() != "::1") was moved to the previous line. It should be in a separate line.

There are no tests for new code introduced in iface_mgr.{cc|h}. In particular, closeSockets() and getSockets() should be tests (easy) and the change about broadcast address in getLocalAddress() (may be tricky). It would be good to have tests for IfaceMgr::openSocketFromRemoteAddress() that check the broadcast path. One possible thing to consider here is to use the same approach BIND9 is using. They have a script that is expected to run (with root privileges) before tests. That script sets some well known addresses on the loopback interface. Then the tests check if the expected addresses are there, then skip some cases if they aren't. All BIND9 developers had run this script once and they now have some specific address setup on their machines and can run those additional tests. Of course, if you can come up with a simpler approach, that would be preferable.

src/lib/dhcp/iface_mgr.h
Nit-pick: The comment for closeSockets() should end with a dot.

The comment for getSockets() should caution that the returned reference is only valid as long as the object that returned it. It was introduced in commit 4ae9b5ea.

Since SocketsCollection? is now used outside of the Iface class, perhaps SocketsCollectionIter? type should be defined? I don't have strong opinion here, so it may be left as it is now.

Documentation
I see that there's quite a bit of new code in the iface_mgr (added mostly in previous tickets). Some description about how the socket management is done should be added to doc/devel/02-dhcp.dox. In fact, the 20-dhcp.dox can probably be renamed, split and perhaps moved to more suitable directory. Or it can stay in doc/devel - it is up to you.

The key point with the documentation here is that not huge multi-page is needed. It is addressed to C++ developers, who can read the code and understand what each method does separately. It should present "the big picture".

libdhcp++.cc
I just wanted to say that it is great that the code is being written there. It will be very useful for option definition framework.

What happens if the buffer passed to optionFactory() is not consumed entirely, e.g. there are 20 bytes for an option that conveys a single IPv4 address? There's no way to detect that or report somehow where the parsing ended.

A packet is received and put into some buffer. How is this mechanism going to be used without copying the data to a separate OptionBuffer?, just to call optionFactory() that will create an object that will copy the data again (that copy is unavoidable)?

libdhcp++.h
There is an extra empty line after optionFactory(). Methods should be separated with a single line only.

option.h
There is an extra empty line after factory() method.

tests/libdhcp++_unittest.cc
The test for optionFactory() is not thorough enough. Dummy function "return OptionPtr?(1);" would have passed it. Additional checks for returned option being the right type of right length and having the right content is needed.

Review of changes in tests/tools/perfdhcp will follow.

comment:7 follow-up: Changed 7 years ago by tomek

tests/tools/perfdhcp/command_options.h
Comment for initialize() method: Initializes class members based [on the] command line. There are missing dots at the end of many doxygen comments.

Why is the generateDuidPrefix() function contains prefix in its name? It generates the whole DUID, not just prefix. Prefix has no meaning in the DUID context. As prefix has specific meaning in DHCP context, it is better to avoid ambiguity here. Perhaps it would be better to use "template" instead?

generateDuidPrefix() uses 6 byte long MACs. That is ok for now, but it should be extended eventually. The new DUID type DUID-UUID is more than 14 octets long. RFC3315 states that DUIDs can be up to 128 bytes long. We should test it one day. I don't suggest that we should implement it now, but adding some TODO comment is reasonable.

CommandOptions::printCommandLine() should also print specified command-line parameters as they are. That may seem redundant, but there is very good reason to print them out. It is easy to reproduce the case, based only on log file. It is very useful to keep a format that can be copy-pasted easily into the next execution.

There are no tests for initIsInterface() and generateDuidPrefix().

tests/tools/perfdhcp/localized_option.h
Comments for constructors should advise that options 0 and 255 (v4) and 0 (v6) are not valid. It is still very much ok to allow them for server testing purposes.

Constructors:

    LocalizedOption(dhcp::Option::Universe u,
                    uint16_t type,
                    dhcp::OptionBufferConstIter first,
                    dhcp::OptionBufferConstIter last)

and

    LocalizedOption(dhcp::Option::Universe u,
                    uint16_t type,
                    dhcp::OptionBufferConstIter first,
                    dhcp::OptionBufferConstIter last, const size_t offset)

can be combined, with the default value of the fifth parameter being 0.

Invalid namespace specified in line 172. It is isc::perfdhcp, not perfdhcp.

perf_pkt4.cc and perf_pkt6.cc
Methods writeAt() and writeValueAt are not tested.

pkt_transform.cc
PktTransform::writeAt() uses inefficient byte-by-byte copying. It would be faster to use something like

memcpy(&in_buffer[dst_pos], &(*first), distance(first, last));

This is a performance testing tool - using tradeoffs between clean C++ approach vs raw speed should usually favor speed.

I couldn't find any tests for PktTransform? class. Are there any? I see that it is used by PerfPkt4, PerfPkt6 that are tested, but it would be better to test PktTransform? directly as well.

pkt_transform.h
writeValueAt() is intended to be used for integer types only, right? Aren't there a simpler way to do it?

stats_mgr.h
Why are two opertor++ definitions are needed?

getValue(), getName() and similar - for simple methods no repetition of \brief descriptions are needed, the brief description is sufficient.

PktList? type - I bow to your superior template handling techniques, sir. How many nested templates are there? 6? :) Seriously speaking, I do not understand that PktList? definition.

sum_delay_squared_ field - Is this squared sum of delays or sum of squared delays? Note that it is not the same. If it is the former, then it is not needed and sum_delay_ may be used instead.

getDroppedPacketsNum(): Unless this method is expected to become more complex in the future, the drops variable is not needed. You ca use:

if (getSentPacketsNum() > getRcvdPacketsNum()) {
    return (getSentPacketsNum() - getRcvdPacketsNum());
} else {
    return (0);
}

printIntermediateStats(): ostringstream objects don't have to be explicitly initialized to empty strings. The default constructor does that as well, but is faster. ++it in "for" loop doesn't have to be written in a separate line.

Review finished on line 1502 of the patch. It is after 8pm Friday night. The review will be continued on Monday.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by marcin

Replying to tomek:

First, let me say that this ticket is huge (the diff is 5400 lines long). In future tickets, it would be reasonable to split the work into separate tickets to make it more manageable.

Yes, I am sorry for this. I will do my best to avoid this in the future.

src/lib/dhcp/iface_mgr.cc
Please remove cout statement in closeSockets(). There's a separate ticket for removing cout in the whole libdhcp library. Let's not add new work to it.

Removed

getLocalAddress()
How is the code for broadcast is going to work on a device with multiple interfaces? 255.255.255.255 is a local network broadcast address. If your host is connected to 2 networks, it is valid on both. There should be a way to specify which interface is going to be used. I'm not sure how to do it with boost (and IPv4), but there is a field in6_scope_id that can be used for scope in IPv6. Perhaps something similar is available for IPv4. If it isn't, we should at least mention it in the docs. (od add @todo note somewhere).

openSocket6()
if (addr.toText() != "::1") was moved to the previous line. It should be in a separate line.

Corrected.

There are no tests for new code introduced in iface_mgr.{cc|h}. In particular, closeSockets() and getSockets() should be tests (easy) and the change about broadcast address in getLocalAddress() (may be tricky). It would be good to have tests for IfaceMgr::openSocketFromRemoteAddress() that check the broadcast path. One possible thing to consider here is to use the same approach BIND9 is using. They have a script that is expected to run (with root privileges) before tests. That script sets some well known addresses on the loopback interface. Then the tests check if the expected addresses are there, then skip some cases if they aren't. All BIND9 developers had run this script once and they now have some specific address setup on their machines and can run those additional tests. Of course, if you can come up with a simpler approach, that would be preferable.

I implemented multipleSockets test that combines testing of getSockets() and closeSockets(). It will basically try to open more than one socket, check if list of sockets is matching expected socket descriptors. It will also try to use sockets (expecting success), close them with closeSockets() and try to use them again (expecting failure).

In the socketFromRemoteAddress I added new section to test broadcast address like this:

 int socket3  = 0;
    IOAddress bcastAddr("255.255.255.255");
    EXPECT_NO_THROW(
        socket3 = ifacemgr->openSocketFromRemoteAddress(bcastAddr, PORT2);
    );
    EXPECT_GT(socket3, 0);
    close(socket3);

src/lib/dhcp/iface_mgr.h
Nit-pick: The comment for closeSockets() should end with a dot.

Dot added.

The comment for getSockets() should caution that the returned reference is only valid as long as the object that returned it. It was introduced in commit 4ae9b5ea.

Added extra comment.

Since SocketsCollection? is now used outside of the Iface class, perhaps SocketsCollectionIter? type should be defined? I don't have strong opinion here, so it may be left as it is now.

I left it untouched for now. Writing SocketCollection::iterator or SocketCollection::const_iterator does not cost a lot comparing to SocketCollectionIter?. Assuming that objects in SocketCollection? are rather read only, clients should use const_iterator. Shouldn't it be SocketCollectionConstIter? defined instead?

Documentation
I see that there's quite a bit of new code in the iface_mgr (added mostly in previous tickets). Some description about how the socket management is done should be added to doc/devel/02-dhcp.dox. In fact, the 20-dhcp.dox can probably be renamed, split and perhaps moved to more suitable directory. Or it can stay in doc/devel - it is up to you.

The key point with the documentation here is that not huge multi-page is needed. It is addressed to C++ developers, who can read the code and understand what each method does separately. It should present "the big picture".

It is planned that we create an additional ticket to update documentation of perfdhcp after code refactoring and integration. I suggest that I implement your suggestion as a part of this new ticket.

libdhcp++.cc
I just wanted to say that it is great that the code is being written there. It will be very useful for option definition framework.

What happens if the buffer passed to optionFactory() is not consumed entirely, e.g. there are 20 bytes for an option that conveys a single IPv4 address? There's no way to detect that or report somehow where the parsing ended.

A packet is received and put into some buffer. How is this mechanism going to be used without copying the data to a separate OptionBuffer?, just to call optionFactory() that will create an object that will copy the data again (that copy is unavoidable)?

I did not want to complicate this new factory function. I assumed that the purpose of this function is to find if the appropriate factory function has been registered and pass the call to it.
The necessity to copy the buffers occurs because of Option class constructor that takes the reference to the buffer. This buffer has to be created somewhere. Usually it will be created within the custom factory function or by the client class that calls Option::Factory(). Since it is passed by reference further on, it will not be copy constructed any further until Option constructor that initializes data_ member. In fact we have just one copy operation that we can't really avoid.

libdhcp++.h
There is an extra empty line after optionFactory(). Methods should be separated with a single line only.

Removed empty line.

option.h
There is an extra empty line after factory() method.

Removed empty line.

tests/libdhcp++_unittest.cc
The test for optionFactory() is not thorough enough. Dummy function "return OptionPtr?(1);" would have passed it. Additional checks for returned option being the right type of right length and having the right content is needed.

I added requested checks to the test.

Review of changes in tests/tools/perfdhcp will follow.

comment:9 follow-up: Changed 7 years ago by tomek

This review is based on commit-id 6efd035f6178974adb48ace599f6d2335c0afd7e (and all previous changes on trac1959 that are not present on master).

Generic comment: CommandOptions? class should be renamed to CommandParameters? or CommandSwitches?. Option has very specific meaning in DHCP context - something different that CommandOptions? convey. This applies to many uses of that class, e.g. in sendRequest6() and similar methods, "options" object is really for DHCP options. It should be named "parameters" (or "params" if you prefer short names).

tests/tools/perfdhcp/test_control.h

Is the TestControlSocket? class really needed? Why can't you use IfaceMgr::SocketInfo?? It holds all the data, except interface information. If you need interface info, perhaps TestControlSocket? could be derived from IfaceMgr::SocketInfo?? Its definition would be simpler.

Private default TestControlSocket? constructor is not need. Since there is another constructor explicitly defined, the implicit default constructor with public access will not be created.

~TestControlSocket?(): There's TODO outstanding there - to close only the socket represented by this object, not all sockets in the whole IfaceMgr?. Fortunately, that is very easy to do. The following code should do the trick:

IfaceMgr::Iface* iface = IfaceMgr::instance().getIface(ifindex_);
if (iface) {
    iface->delSocket(socket_);
}

Also, TODO should be written as \TODO or @TODO to mark it up on Doxygen todo list.

HW_ETHER_LEN constant: it is ok to keep it as 6 bytes for now, but we will need to extend the code one day to check if longer MAC addresses cause problems. The longest link-layer address I'm aware of is used in Infiniband. It is 20 bytes long. A simple @TODO will do the trick for now.

TestControlSocket? keeps both interface name (iface_) and its index (ifindex_). Keeping both is redundant. If both are kept for the (minimal) performance reasons, it should be commented. Otherwise it would be simpler to keep a pointer to the interface object and use its getName() and getIndex(). The added benefit is an access to getFullName() method that is useful for logging purposes (it returns both in a readable format, e.g. "eth0/2"). It is also more extensive, if you ever decide that more information is needed in the future.

The comment just after protected: It should be moved above protected. The general approach is that the comment appears before the piece of code that is being commented. It is a good point about using friends. I tried to use that approach in one ticket, but I later removed it to follow whatever others do. It might be useful to write a question about it to bind10-dev asking if people are ok with using "friends" approach. As the discussion on bind10-dev may be lengthy, carry on with this ticket. If the discussion reaches a conclusion that fried is the way to go, we will have lots of places to change the code anyway.

checkExitConditions() description: fulfiled => fulfilled.

factoryElapsedTime6(): buf parameter should be described more throughly - does it contain the whole option (including option header, 6 bytes in total) or just content of the option(2 bytes)? This also applies to many other factory methods.

This factory is supposed to create *DHCPv6* option. Why does it take universe parameter? If this parameter is necessary because of factory registration, then it should be commented on (and definitely not say V4 or V6 in the parameter description).

factoryIana6(), factoryOptionRequestOption6(): Why those methods end with 6? There are no v4 equivalents of those options, so 6 is redundant in the name.

Many factory methods throw exceptions if specified buffer is of invalid size. That should be reflected in the description with \throw.

generateDuid(): typo in the description: clinets => clients (x2). uint8_t passed by reference, because this method sets the randomized value. It should be noted somewhere in the description that this is an output parameter and its initial value is ignored.

generateMacAddress(): See generateDuid() comments.

generateTransId(): should mention if the transid is 32bit value (DHCPv4) or (DHCPv6).

getNextExchangesNum(): immediatelly => immediately.

getTemplateBuffer(): Wouldn't it be more correct to throw exception, when user requested non-existing buffer?

initTemplateFiles(): Where is the list of template files specified? It should briefly comment on it, as that info is not passed as parameter.

openSocket(): Why do you need to set multicast() flag on the socket for DHCPv6? This flag is only needed for receiving multicast traffic. A client never never does that. With v4, things are a bit different, as the server response may be sent to broadcast address if certain conditions are met.

The comment for open socket should also mention if the sockets opened are registered in IfaceMgr? or not. I presume they are, otherwise IfaceMgr? wouldn't be able to use them to send and receive traffic.

There are many articles missing in the comments in test_control.h. As I've never learned to handle them correctly, I will not bother to guess when they're needed.

receivePacket4(), receivePacket6(): It looks more natural when received packet is returned by the method, so unless there are specific reasons to do otherwise, I would prefer the prototype to be:

const dhcp::Pkt4Ptr receivePacket4(const TestControlSocket& socket);

On the other hand, if you want to keep it similar to sendXXX4/6() methods, I won't object, but please add [in] or [out] to the parameters passed by reference.

Also, both methods should throw if someone tries to use v4 socket to receive v6 packet (and vice versa). That would be easy to check if IfaceMgr::SocketInfo? structures were used. If such a check is not desired, because it is already checked in receivePackets(), a warning comment should be added.

sendDiscover4(), sendSolicit6(), sendRequest4() and many similar:

  • for parameters passed by reference, please specify the direction ([in], [out] or [in,out]). See http://www.stack.nl/~dimitri/doxygen/docblocks.html (search for [in]).
  • please supplement the descriptions with the information that sent packets are stored somewhere (in stats_mgr).

testDiags(): Find of => Find if

byte2Hex(), vector2Hex(): These are very generic methods. Similar methods exist in src/lib/util/encode/hex.h (and other headers in that directory). If there isn't one already defined, please move them there.

getElapsedTime(). This method is sufficiently complex to move its body to .cc file. Also, wouldn't it be more appropriate to throw exception (or return 0xffffffff) when time is not specified? The reason for returning 0xffffffff is to mark bogus packet exchanges. (there's definitely something wrong if packet was not timestamped).

factoryIana6(): The description should be very specific that the option-buffer should point to IA_NA *sub-options*. The code should have @todo somewhere stating that IAID, T1 and T2 are hardcoded. Perhaps .cc file is better for such @todo comment.

tests/tools/perfdhcp/test_control.cc

General comment: Adding "using namespace XYZ;" would make many expressions shorter.

checkExitConditions():148. The check should be

if (options.getNumRequests().size() > 1) {

as a future compatibility for cases, when RENEW support will be added.

The check for reaching maximum drops percentage can be skipped, if total drop is 0. A simple if around the whole section should do the trick.

factoryElapsedTime6(): As a sanity check we should probably verify that universe is v6 and option type is D6O_ELAPSED_TIME. The same applies to other factory functions.

factoryOptionRequestOption6(): The comment in .h file states that the buf should be empty and is ignored. There should be a check whether it is indeed empty. Or even better, it should add name server and domain search as it does now only if the buffer is empty, otherwise treat the buffer as list of options to request. You may mark this as @todo for a future work if you don't want to implement it now.

factoryRequestList4(): See factoryOptionRequestOption6().

generateMacAddress(),generate: The comment states that the random number must be in range 0..clients_num, but the code will result in 0..clinets_num - 1. Either the code or comment should be updated.

generateDuid(): please use STL copy function to copy mac_addr to duid, instead of copying byte by byte.

getNextExchangesNum(): If you want to make sure that at least one packet goes out, you should use

if (due_exchanges == 0) {
  due_exchanges == 1;
}

or any fancy way of doing exactly that. Simply increasing it by one may overshoot and in the worst case double due_exchanges value.

getRcvdPacketsNum(), getSentPacketsNum(), getTemplateBuffer(): Some people would say that the second return should be in else clause. I personally like the style you used as it keeps the code less indented.

initPacketTemplates(): ++it does not have to be on a separate line.

There are many cases were the code for stats_mgr 4 and 6 are repeated. Have you considered making stats_mgr4 and stats_mgr6 derived from a common ancestor and using a single pointer? That would simplify the code a lot.

openSocket(): Please use DHCP4_{SERVER|CLIENT}_PORT. I have no idea why 68 is "wrong" there. It should work.

run(): Diagnostics is => Diagnostics are

When options.isSeeded() is called you seed PRNG with the specified seed. Otherwise you should seed it with current time (or if it is already seeded somewhere, add a comment to point out specific location).

ourselfs => ourselves

template_idx is always zero. Are there any cases when additional templates may be used? If not, please add an appropriate comment.

Why "interrupted" is printed when 'e' flag is set? It seems there may be other reasons to exit the main loop.

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.

"Set elapsed time" needs one more indent space.

sendRequest6(): Elapsed time is calculated wrong. In DHCPv4 secs field is specified in number of seconds since address acquisition has started. However, in DHCPv6 it specifies how much time elapsed since client initiated *current* transaction (i.e. for REQUEST the time is measured from first transmission of REQUEST message, not first SOLICIT).

There is no check if received ADVERTISE is sane or not. First, the code should check if there is STATUS_CODE option in the message with non-zero status. Then it should check for non-zero STATUS_CODE opion in the IA_NA option. Finally it should check if there's IAADDR option in IA_NA. If you don't want to implement this now, just adding appropriate @TODO will be sufficient for now.

updateSendDue(): Is the code really that fast that it usually send out packeted within microsecond of when it was supposed to be sent? I would add a little delta when comparing now and send_due_ to avoid cases when things are happening at the boundary between n-th and n-th microsecond. Some time later, this could be made a configurable parameter ("allowed send delay" or "send delay tolerance"), but it is ok to add @todo for now (if you think it is a good idea to implement such a parameter).

This concludes test_control.{cc|h}. I will continue looking at the remainder of the changes tomorrow.

Note to self: the review will resume from diff line 3892.

comment:10 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by marcin

Replying to tomek:

tests/tools/perfdhcp/command_options.h
Comment for initialize() method: Initializes class members based [on the] command line. There are missing dots at the end of many doxygen comments.

Comments corrected.

Why is the generateDuidPrefix() function contains prefix in its name? It generates the whole DUID, not just prefix. Prefix has no meaning in the DUID context. As prefix has specific meaning in DHCP context, it is better to avoid ambiguity here. Perhaps it would be better to use "template" instead?

I have taken naming convention from old perfdhcp implementation but after thinking that through again I admit that "Template" sounds better here. Functions have been renamed.

generateDuidPrefix() uses 6 byte long MACs. That is ok for now, but it should be extended eventually. The new DUID type DUID-UUID is more than 14 octets long. RFC3315 states that DUIDs can be up to 128 bytes long. We should test it one day. I don't suggest that we should

implement it now, but adding some TODO comment is reasonable.

I added \todo comments in the header files.

CommandOptions::printCommandLine() should also print specified command-line parameters as they are. That may seem redundant, but there is very good reason to print them out. It is easy to reproduce the case, based only on log file. It is very useful to keep a format that can be copy-pasted easily into the next execution.

Good suggestion. Tool now always prints the command line as it was typed in.

There are no tests for initIsInterface() and generateDuidPrefix().

These functions are not called explicitely but they are called by the CommandOptions? internal logic when it parses command line. Please see added comments in checkDefaults() and
Interface test.

tests/tools/perfdhcp/localized_option.h
Comments for constructors should advise that options 0 and 255 (v4) and 0 (v6) are not valid. It is still very much ok to allow them for server testing purposes.

I added comments.

Constructors:

    LocalizedOption(dhcp::Option::Universe u,
                    uint16_t type,
                    dhcp::OptionBufferConstIter first,
                    dhcp::OptionBufferConstIter last)

and

    LocalizedOption(dhcp::Option::Universe u,
                    uint16_t type,
                    dhcp::OptionBufferConstIter first,
                    dhcp::OptionBufferConstIter last, const size_t offset)

can be combined, with the default value of the fifth parameter being 0.

Good catch. I removed redundant definitions.

Invalid namespace specified in line 172. It is isc::perfdhcp, not perfdhcp.

Corrected.

perf_pkt4.cc and perf_pkt6.cc
Methods writeAt() and writeValueAt are not tested.

I added tests for these methods and .... I found bug in writeValueAt. Thanks for this! :-)

pkt_transform.cc
PktTransform::writeAt() uses inefficient byte-by-byte copying. It would be faster to use something like

memcpy(&in_buffer[dst_pos], &(*first), distance(first, last));

This is a performance testing tool - using tradeoffs between clean C++ approach vs raw speed should usually favor speed.

Fixed.

I couldn't find any tests for PktTransform? class. Are there any? I see that it is used by PerfPkt4, PerfPkt6 that are tested, but it would be better to test PktTransform? directly as well.

PktTransform? does not have any dedicated tests but as you have noticed all functions are directly called by PerfPkt4 and PerfPkt6 with no additional logic. Client classes would use PerfPkt4 and PerfPkt6 rather than PktTransform? so it makes sense in my opinion to focus on PerfPktX unit testing. In the same time, adding tests for both would produce a redundancy.

pkt_transform.h
writeValueAt() is intended to be used for integer types only, right? Aren't there a simpler way to do it?

They are integers only. What is the simpler way?

stats_mgr.h
Why are two opertor++ definitions are needed?

I wanted to have ability to do either ++counter and counter++. In fact just ++counter is the one used in the code but I don't see any problem with having two operators just in case counter++ becomes needed some time.

getValue(), getName() and similar - for simple methods no repetition of \brief descriptions are needed, the brief description is sufficient.

I will do these changes in the course of #1960. This ticket will be mostly to clean up the code.

PktList? type - I bow to your superior template handling techniques, sir. How many nested templates are there? 6? :) Seriously speaking, I do not understand that PktList? definition.

I don't like having so many nested templates but in fact boost is about templates and some of them may become quite complex in use. The multi_index_container is documented here:
http://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/index.html
I also added some more comments in the typedef.

sum_delay_squared_ field - Is this squared sum of delays or sum of squared delays? Note that it is not the same. If it is the former, then it is not needed and sum_delay_ may be used instead.

It is sum of squared delay values. I think I called that squared_sum_delay_ initially but Stephen pointed out that it is wrong name because it suggests that first we add non-squared values and then square it. I renamed it then per Stephen's suggestion to sum_delay_squared_ which is more appropriate. In this case you can see that there is no way to use sum_delay_ instead.

getDroppedPacketsNum(): Unless this method is expected to become more complex in the future, the drops variable is not needed. You ca use:

if (getSentPacketsNum() > getRcvdPacketsNum()) {
    return (getSentPacketsNum() - getRcvdPacketsNum());
} else {
    return (0);
}

That's right but I prefer having less indented code when possible. I have many places like this in the code. I think there is too much overhead now to change them all to the style you're proposing.

printIntermediateStats(): ostringstream objects don't have to be explicitly initialized to empty strings. The default constructor does that as well, but is faster. ++it in "for" loop doesn't have to be written in a separate line.

Corrected. Why are the default constructors faster here?

Review finished on line 1502 of the patch. It is after 8pm Friday night. The review will be continued on Monday.

Thank you for the useful comments.

comment:11 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by marcin

Replying to tomek:

This review is based on commit-id 6efd035f6178974adb48ace599f6d2335c0afd7e (and all previous changes on trac1959 that are not present on master).

Generic comment: CommandOptions? class should be renamed to CommandParameters? or CommandSwitches?. Option has very specific meaning in DHCP context - something different that CommandOptions? convey. This applies to many uses of that class, e.g. in sendRequest6() and similar methods, "options" object is really for DHCP options. It should be named "parameters" (or "params" if you prefer short names).

I am not fully convinced but I think we can do this as a part of code cleanup (ticket #1960).

tests/tools/perfdhcp/test_control.h

Is the TestControlSocket? class really needed? Why can't you use IfaceMgr::SocketInfo?? It holds all the data, except interface information. If you need interface info, perhaps TestControlSocket? could be derived from IfaceMgr::SocketInfo?? Its definition would be simpler.

I prefer classes with read-only access to variables and that's why I started implementing my own class here. However I admit that it is cleaner to reuse the existing code so I now derive from SocketInfo?.

Private default TestControlSocket? constructor is not need. Since there is another constructor explicitly defined, the implicit default constructor with public access will not be created.

Ok, removed.

~TestControlSocket?(): There's TODO outstanding there - to close only the socket represented by this object, not all sockets in the whole IfaceMgr?. Fortunately, that is very easy to do. The following code should do the trick:

IfaceMgr::Iface* iface = IfaceMgr::instance().getIface(ifindex_);
if (iface) {
    iface->delSocket(socket_);
}

Ok, I changed the destructor as suggested.

Also, TODO should be written as \TODO or @TODO to mark it up on Doxygen todo list.

Fixed.

HW_ETHER_LEN constant: it is ok to keep it as 6 bytes for now, but we will need to extend the code one day to check if longer MAC addresses cause problems. The longest link-layer address I'm aware of is used in Infiniband. It is 20 bytes long. A simple @TODO will do the trick for now.

I added todo statement.

TestControlSocket? keeps both interface name (iface_) and its index (ifindex_). Keeping both is redundant. If both are kept for the (minimal) performance reasons, it should be commented. Otherwise it would be simpler to keep a pointer to the interface object and use its getName() and getIndex(). The added benefit is an access to getFullName() method that is useful for logging purposes (it returns both in a readable format, e.g. "eth0/2"). It is also more extensive, if you ever decide that more information is needed in the future.

I left ifindex_ and now client class uses IfaceMgr::getIface to get the interface name.

The comment just after protected: It should be moved above protected. The general approach is that the comment appears before the piece of code that is being commented. It is a good point about using friends. I tried to use that approach in one ticket, but I later removed it to follow whatever others do. It might be useful to write a question about it to bind10-dev asking if people are ok with using "friends" approach. As the discussion on bind10-dev may be lengthy, carry on with this ticket. If the discussion reaches a conclusion that fried is the way to go, we will have lots of places to change the code anyway.

Comment moved. That's going to be hard job to change all classes to use "friend" approach. Maybe this could be applied to new classes only.

checkExitConditions() description: fulfiled => fulfilled.

Corrected.

factoryElapsedTime6(): buf parameter should be described more throughly - does it contain the whole option (including option header, 6 bytes in total) or just content of the option(2 bytes)? This also applies to many other factory methods.

Comment added.

This factory is supposed to create *DHCPv6* option. Why does it take universe parameter? If this parameter is necessary because of factory registration, then it should be commented on (and definitely not say V4 or V6 in the parameter description).

The parameter is required for registration purpose. With current approach to register factory functions many parameters like this will be ignored. I updated comments at parameters which are ignored that they are ignored.

factoryIana6(), factoryOptionRequestOption6(): Why those methods end with 6? There are no v4 equivalents of those options, so 6 is redundant in the name.

The naming convention that I am following is use

  • factoryABC6 for functions that are used to create DHCPv6 options only
  • factoryDEF4 for functions that are used to create DHCPv4 options only
  • factoryGHI for functions that can be used for either DHCPv4 or v6 options.

The function names correspond to this. Although it may seem a little redundant the option name indicates which factory function is for which IP mode (for somebody who does not recognize it from the option name). I insist that we leave it as it is.

Many factory methods throw exceptions if specified buffer is of invalid size. That should be reflected in the description with \throw.

I belive it is. Can you point which one that throws exception is left without \throw?

generateDuid(): typo in the description: clinets => clients (x2). uint8_t passed by reference, because this method sets the randomized value. It should be noted somewhere in the description that this is an output parameter and its initial value is ignored.

Comment updated.

generateMacAddress(): See generateDuid() comments.

Comment updated.

generateTransId(): should mention if the transid is 32bit value (DHCPv4) or (DHCPv6).

Comment updated.

getNextExchangesNum(): immediatelly => immediately.

Fixed.

getTemplateBuffer(): Wouldn't it be more correct to throw exception, when user requested non-existing buffer?

It now throws exception. It does not change anything but it is more consistent with other functions at least.

initTemplateFiles(): Where is the list of template files specified? It should briefly comment on it, as that info is not passed as parameter.

I am not sure if I understand your point. The list of template files is given from command line. I updated comments to reflect this.

openSocket(): Why do you need to set multicast() flag on the socket for DHCPv6? This flag is only needed for receiving multicast traffic. A client never never does that. With v4, things are a bit different, as the server response may be sent to broadcast address if certain conditions are met.

The logic to set multicast option on the socket for multicast remote addresses is taken from previous perfdhcp version (implemented by Francis). I will double check if I can safely remove this code during the course of #1960.

The comment for open socket should also mention if the sockets opened are registered in IfaceMgr? or not. I presume they are, otherwise IfaceMgr? wouldn't be able to use them to send and receive traffic.

I added this comment but from the caller perspective it is of a less importance. This is more TestControl? class internals that rely on this fact.

There are many articles missing in the comments in test_control.h. As I've never learned to handle them correctly, I will not bother to guess when they're needed.

I added one or two articles. I haven't found "many".

receivePacket4(), receivePacket6(): It looks more natural when received packet is returned by the method, so unless there are specific reasons to do otherwise, I would prefer the prototype to be:

const dhcp::Pkt4Ptr receivePacket4(const TestControlSocket& socket);

The packet is NOT returned here at all. It is the input parameter. The actual packet reception from the wire is done elsewhere. The receivePacketX functions simply do processing on received packet (e.g. update statistics on StatsMgr?, calculate RTT etc.).

On the other hand, if you want to keep it similar to sendXXX4/6() methods, I won't object, but please add [in] or [out] to the parameters passed by reference.

I put the [in,out] where applicable. I did not put this where I pass const reference or where I don't pass reference at all.

Also, both methods should throw if someone tries to use v4 socket to receive v6 packet (and vice versa). That would be easy to check if IfaceMgr::SocketInfo? structures were used. If such a check is not desired, because it is already checked in receivePackets(), a warning comment should be added.

The code checks mode of operation (v4 or v6) on the very beginning and uses receive4 or receive6 methods accordingly. If the code is ok (I belive it is) it should never get v6 packet on v4 socket etc. I added warnings to receivePacketX functions that they don't do these sanity checks.

sendDiscover4(), sendSolicit6(), sendRequest4() and many similar:

Why would I need it here? I pass all variables by const reference. It is clear that they are [in] params.

  • please supplement the descriptions with the information that sent packets are stored somewhere (in stats_mgr).

Comment added?

testDiags(): Find of => Find if

Corrected.

byte2Hex(), vector2Hex(): These are very generic methods. Similar methods exist in src/lib/util/encode/hex.h (and other headers in that directory). If there isn't one already defined, please move them there.

My methods in their shape do not fit to src/lib/util/encode/hex.h or any other file there. In the same time, methods provided there do not provide separators. Trying to move my code there might be tricky and I prefer postponing it for now.

getElapsedTime(). This method is sufficiently complex to move its body to .cc file. Also, wouldn't it be more appropriate to throw exception (or return 0xffffffff) when time is not specified? The reason for returning 0xffffffff is to mark bogus packet exchanges. (there's definitely something wrong if packet was not timestamped).

Moved to .cc file and exception is now thrown.

factoryIana6(): The description should be very specific that the option-buffer should point to IA_NA *sub-options*. The code should have @todo somewhere stating that IAID, T1 and T2 are hardcoded. Perhaps .cc file is better for such @todo comment.

Added comment.

tests/tools/perfdhcp/test_control.cc

General comment: Adding "using namespace XYZ;" would make many expressions shorter.

But in many cases having namespaces is preferred because it clearly indicates which module the methods belong to.

checkExitConditions():148. The check should be

if (options.getNumRequests().size() > 1) {

as a future compatibility for cases, when RENEW support will be added.

Ok. I added this but if we ever enable renew support it will have to be modified anyway.

The check for reaching maximum drops percentage can be skipped, if total drop is 0. A simple if around the whole section should do the trick.

Does it really improve any performance here? I don't want this function to grow any bigger than necessary so I dropped this suggestion.

factoryElapsedTime6(): As a sanity check we should probably verify that universe is v6 and option type is D6O_EeLAPSED_TIME. The same applies to other factory functions.

The factory functions now ignore universe.

factoryOptionRequestOption6(): The comment in .h file states that the buf should be empty and is ignored. There should be a check whether it is indeed empty. Or even better, it should add name server and domain search as it does now only if the buffer is empty, otherwise treat the buffer as list of options to request. You may mark this as @todo for a future work if you don't want to implement it now.

I changed the comment. The buffer is ignored.

factoryRequestList4(): See factoryOptionRequestOption6().

Same here.

generateMacAddress(),generate: The comment states that the random number must be in range 0..clients_num, but the code will result in 0..clinets_num - 1. Either the code or comment should be updated.

I updated the comment.

generateDuid(): please use STL copy function to copy mac_addr to duid, instead of copying byte by byte.

Change to std::copy.

getNextExchangesNum(): If you want to make sure that at least one packet goes out, you should use

if (due_exchanges == 0) {
  due_exchanges == 1;
}

or any fancy way of doing exactly that. Simply increasing it by one may overshoot and in the worst case double due_exchanges value.

Ok. I added this condition.

getRcvdPacketsNum(), getSentPacketsNum(), getTemplateBuffer(): Some people would say that the second return should be in else clause. I personally like the style you used as it keeps the code less indented.

So we are on the same side. Great :-)

initPacketTemplates(): ++it does not have to be on a separate line.

It is not anymore.

There are many cases were the code for stats_mgr 4 and 6 are repeated. Have you considered making stats_mgr4 and stats_mgr6 derived from a common ancestor and using a single pointer? That would simplify the code a lot.

o
This sounds reasonably. Initially I did not think that there will be so many distinct invocations to StatsMgr?<Pkt4> and StatsMgr?<Pkt6>. In the future we may reconsider this but not as a part of this ticket. I will take a look at this as a part of #1960.

openSocket(): Please use DHCP4_{SERVER|CLIENT}_PORT. I have no idea why 68 is "wrong" there. It should work.

This will be handled in #1960.

run(): Diagnostics is => Diagnostics are

Fixed.

When options.isSeeded() is called you seed PRNG with the specified seed. Otherwise you should seed it with current time (or if it is already seeded somewhere, add a comment to point out specific location).

I now seed from current time.

ourselfs => ourselves

Fixed.

template_idx is always zero. Are there any cases when additional templates may be used? If not, please add an appropriate comment.

The reason to define the constant is that everybody knows that 0 is template index. I do another constant when I get the template for request packet. I just avoid using unnamed '0's in the code. I added some comment there.

Why "interrupted" is printed when 'e' flag is set? It seems there may be other reasons to exit the main loop.

The other reasons (like fatal) error are also printed. Fatal error for example is printed in main.cc.

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.

This will be a part of #1960.

"Set elapsed time" needs one more indent space.

Fixed.

sendRequest6(): Elapsed time is calculated wrong. In DHCPv4 secs field is specified in number of seconds since address acquisition has started. However, in DHCPv6 it specifies how much time elapsed since client initiated *current* transaction (i.e. for REQUEST the time is measured from first transmission of REQUEST message, not first SOLICIT).

Ok. I now set elapsed time == 0 for DHCPv6 requests.

There is no check if received ADVERTISE is sane or not. First, the code should check if there is STATUS_CODE option in the message with non-zero status. Then it should check for non-zero STATUS_CODE opion in the IA_NA option. Finally it should check if there's IAADDR option in IA_NA. If you don't want to implement this now, just adding appropriate @TODO will be sufficient for now.

We will need some other ticket for this to guarantee robustness of this code but also in many other places.

updateSendDue(): Is the code really that fast that it usually send out packeted within microsecond of when it was supposed to be sent? I would add a little delta when comparing now and send_due_ to avoid cases when things are happening at the boundary between n-th and n-th microsecond. Some time later, this could be made a configurable parameter ("allowed send delay" or "send delay tolerance"), but it is ok to add @todo for now (if you think it is a good idea to implement such a parameter).

In fact, I have checked that it is often that we exceed due time and latesend counter is increased. First I would like to implement milisecond timeout in ifaceMgr as it will change a lot the timings in main loop. After that I will take a look how it affects the section you're poiting at.

This concludes test_control.{cc|h}. I will continue looking at the remainder of the changes tomorrow.

Thanks for this part of review.

Note to self: the review will resume from diff line 3892.

comment:12 follow-up: Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

tests/tools/perfdhcp/tests/command_options_helper.h
represinting => representing

CommandOptionsHelper? seems like a useful class that could be shared between other components. dhcp4 and dhcp6 come to mind, but I'm sure there are others. You should investigate, if it is feasible to move it to src/lib/testutils.

In the following code in CommandOptionsTest?.Base:

    // "t" is invalid character in DUID
    EXPECT_THROW(process("perfdhcp -6 -l ethx -b "
                         "duiD=010101010101010101t110111F14 all"),
                 isc::InvalidParameter);

is the duid capitalization (duiD) made on purpose?

Is the parsing case-insensitive? I assume it is, but there seem to be no test for it. For parameters longer than single character, please check at least couple combinations, -Duid= -DuId?= -DUID= etc.

What about odd length DUID, e.g. 01234? Is it padded with a leading zero? Please test it.

What about too short DUIDs, e.g. 12? That should be allowed for server testing purposes, but a warning should be added that DUID not-conformant to 3315 is used. If it is not allowed in the current code, an appropriate @TODO should be added. (it is minor and time should not be spent of implementing such a support now if it is missing).

Please make sure that you test CommandOptionsTest?.Interface on several non-linux systems. IfaceMgr? detects interfaces on Linux only. On other systems it uses very simple approach: it tries to find interface index of "lo" interface. If that fails, it repeats with "lo0". This allows detecting loopback interfaces on all systems, except Windows. That is a bizarre OS that do not feature loopback concept. Fortunately, I think nobody pays any attention to this OS in BIND10 context.

We do have plans to implement interface detection on other OSes, but such a ticket is not currently planned. Do you think it would be useful to implement it soon? There is a getifaddrs() interface that seems to be supported on Linux, all BSDs and surprisingly even Solaris 11.

I don't like the fact that if no interfaces are detected that the test does nothing. In my opinion it should fail in such case.

tests/tools/perfdhcp/tests/test_control_unittest.cc
Let me just say that the trick with using in NamedTestControl? is brilliant!

getLocalLoopback(): Loopback interfaces should have IfaceMgr::Iface::flag_loopback_ set to true.

matchRequestedOptions(): layed => laid

unequalOctetPosition(): the comment for parameter random_size lacks parameter name.
The comment for randomization range suggests that it is always 6. If that is the case, why not making is a fixed value? And it is not really needed anyway.

If I understand the method correctly, in its current it will never return anything above 2. The first iteration will reduce the value to something in 0..255 range. The second iteration will set unequal_pos to 2 and break the loop. For that reason it doesn't work for 65537 and above (returns 2 instead of 3).

The code for this method is surprisingly convoluted. If I understand it correctly, this method should return number of bytes that have to be different to accommodate specified number of clients. So what you really want is:

log256(clients_num - 1);

Here are 2 proposals to do this:

    if (clients_num < 2) // degenerated case. There's nothing to randomize
        return 0;

    return 1 + log(clients_num - 1)/log(256);

It is very short, but has the disadvantage of using floating point arithmetic. Another code that is similar to the code under review would be:

    if (!clients_num)
        return 0;
    clients_num--;

    int cnt = 0;
    while (clients_num) {
        clients_num >>= 8;
        ++cnt;
    }

    return cnt;

The advantage here is not using floating point calculations. As this is a test code, I personally prefer simplicity over performance to avoid possible bugs.

On a related note, how does this method correlate to what is happening in generateDuid()? If you have 256 clients, those clients will have unique DUID only if the generateMacAddress() manages to allocate 256 unique numbers.

I would like to see a test that generates 256 DUIDs and verifies that they are indeed unique. They should differ only on a single byte, right?

testDuid(): The conditions checked in testDuid() are suspicious. Why the test passes if the total_dist is 0? It should only pass if all X DUIDs are different.

Something along the lines:

Duid duid[clients_num];
// fill in DUIDs
for (i = 0; i < clients_num; ++i) {
    for (j = i + 1; j<clients_num; ++j) {
        if (duid[i] == duid[j]))
            FAIL();
    }
}

The same comment applies to testMacAddress().

testPkt4Exchange(): reponses => responses

createOffcePkt4(), creatAdvertisePkt6(): doxygen descriptions are missing.

TestControlTest?.Options4,6: Those tests are generic to all options. Most of the test code is generic, only the call to registerOptionFactories() is specific to TestControl?. You should consider splitting those tests and moving generic parts to src/lib/dhcp/ somewhere.

TestControlTest?.Options6: How does the code for requestes_options work? It uses matchRequestedOptions() that compares the requests on byte by byte basis. DHCPv6 options are coded on 2 bytes. matchRequestdOptions6() is needed.

matchRequestedOptions(): break in the loop should be removed. This will detect option code duplicates.

TestControlTest?.Options6: TODO should look like this / @todo to be picked up by Doxygen TODO list generator.

TestControlTest?.PacketTemplates?: The test should verify that the templates were read correctly.

TestControlTest?.RateControl?: Checks for xchgs_num should be a bit more thorough. This is very broad subject, so I proposed to add TODO and don't spend too much time on it now.

Ok. That's it. End of the review.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by marcin

Replying to tomek:

tests/tools/perfdhcp/tests/command_options_helper.h
represinting => representing

Fixed.

CommandOptionsHelper? seems like a useful class that could be shared between other components. dhcp4 and dhcp6 come to mind, but I'm sure there are others. You should investigate, if it is feasible to move it to src/lib/testutils.

I will investigate this in the course of #1960. I just don't want to complicate things more than they are now.

In the following code in CommandOptionsTest?.Base:

    // "t" is invalid character in DUID
    EXPECT_THROW(process("perfdhcp -6 -l ethx -b "
                         "duiD=010101010101010101t110111F14 all"),
                 isc::InvalidParameter);

is the duid capitalization (duiD) made on purpose?

It was but it was at the same time checking if invalid digit ('t') produces exception so if there was an error in parsing duiD string it would not be clear whether exception comes from case sensitive (invalid) treatment of this option or 't' in the middle of DUID. I changed the "duiD" to "duid' in this particular case to avoid this issue.

Is the parsing case-insensitive? I assume it is, but there seem to be no test for it. For parameters longer than single character, please check at least couple combinations, -Duid= -DuId?= -DUID= etc.

There are now a couple of new test cases that cover this.

What about odd length DUID, e.g. 01234? Is it padded with a leading zero? Please test it.

It is not padded. If odd number of digits is entered the parser will throw exception. It is now tested.

What about too short DUIDs, e.g. 12? That should be allowed for server testing purposes, but a warning should be added that DUID not-conformant to 3315 is used. If it is not allowed in the current code, an appropriate @TODO should be added. (it is minor and time should not be spent of implementing such a support now if it is missing).

Current implementation uses MAC generation function to generate unique DUIDs. For this reason I assume that DUID has to be at least 6 octets long. If it is not then exception is thrown. This is now tested.

Please make sure that you test CommandOptionsTest?.Interface on several non-linux systems. IfaceMgr? detects interfaces on Linux only. On other systems it uses very simple approach: it tries to find interface index of "lo" interface. If that fails, it repeats with "lo0". This allows detecting loopback interfaces on all systems, except Windows. That is a bizarre OS that do not feature loopback concept. Fortunately, I think nobody pays any attention to this OS in BIND10 context.

ok.

We do have plans to implement interface detection on other OSes, but such a ticket is not currently planned. Do you think it would be useful to implement it soon? There is a getifaddrs() interface that seems to be supported on Linux, all BSDs and surprisingly even Solaris 11.

I think it would be very useful so as we can run tests on multiple OSes.

I don't like the fact that if no interfaces are detected that the test does nothing. In my opinion it should fail in such case.

I understand your point but I would rather want to have reliable interface detection on all supported OSes. The interface detection should be checked elsewhere (IfaceMgr?). Here I rely on this fact but I keep this safety valve as long as I am not sure if it is 100% reliable and well tested.

tests/tools/perfdhcp/tests/test_control_unittest.cc
Let me just say that the trick with using in NamedTestControl? is brilliant!

Congratulations to somebody who documented Google Test! :-)

getLocalLoopback(): Loopback interfaces should have IfaceMgr::Iface::flag_loopback_ set to true.

Nice! I now use this although I don't like the fact that this field is public.

matchRequestedOptions(): layed => laid

Fixed. There are a couple of words in English that confuse me every time I write them.

unequalOctetPosition(): the comment for parameter random_size lacks parameter name.
The comment for randomization range suggests that it is always 6. If that is the case, why not making is a fixed value? And it is not really needed anyway.

I removed it. It was not needed.

If I understand the method correctly, in its current it will never return anything above 2. The first iteration will reduce the value to something in 0..255 range. The second iteration will set unequal_pos to 2 and break the loop. For that reason it doesn't work for 65537 and above (returns 2 instead of 3).

The code for this method is surprisingly convoluted. If I understand it correctly, this method should return number of bytes that have to be different to accommodate specified number of clients. So what you really want is:

log256(clients_num - 1);

Here are 2 proposals to do this:

    if (clients_num < 2) // degenerated case. There's nothing to randomize
        return 0;

    return 1 + log(clients_num - 1)/log(256);

It is very short, but has the disadvantage of using floating point arithmetic. Another code that is similar to the code under review would be:

    if (!clients_num)
        return 0;
    clients_num--;

    int cnt = 0;
    while (clients_num) {
        clients_num >>= 8;
        ++cnt;
    }

    return cnt;

The advantage here is not using floating point calculations. As this is a test code, I personally prefer simplicity over performance to avoid possible bugs.

Since you approved both options I picked the second one. I don't like floating points. :-)

On a related note, how does this method correlate to what is happening in generateDuid()? If you have 256 clients, those clients will have unique DUID only if the generateMacAddress() manages to allocate 256 unique numbers.

I would like to see a test that generates 256 DUIDs and verifies that they are indeed unique. They should differ only on a single byte, right?

I used random() function to generate "unique" numbers. The point is that they might not be unique in the short period of time because repetitions are possible. We discussed this on jabber and I think what strikes me here is that "if somebody sets he wants to simulate 100 clients and run 100 iterations he might not get 100 leases and he will be unhappy trying to debug why it is the case". My understanding was that if you run the test for a long period of time (BTW, this is how performance should be measured) the number of leases will reach 100 or will be close enough. However, after our discussion I think it is good point to guarantee the unique DUIDs and MACs within number of iterations equal to number of simulated clients. I do it now with sequential number generators.

testDuid(): The conditions checked in testDuid() are suspicious. Why the test passes if the total_dist is 0? It should only pass if all X DUIDs are different.

Ok. I now changed this test quite a lot. it should not be confusing anymore.

Something along the lines:

Duid duid[clients_num];
// fill in DUIDs
for (i = 0; i < clients_num; ++i) {
    for (j = i + 1; j<clients_num; ++j) {
        if (duid[i] == duid[j]))
            FAIL();
    }
}

The same comment applies to testMacAddress().

The test for MAC address generation has been changed as well.

testPkt4Exchange(): reponses => responses

Fixed.

createOffcePkt4(), creatAdvertisePkt6(): doxygen descriptions are missing.

Added.

TestControlTest?.Options4,6: Those tests are generic to all options. Most of the test code is generic, only the call to registerOptionFactories() is specific to TestControl?. You should consider splitting those tests and moving generic parts to src/lib/dhcp/ somewhere.

I don't agree. I have a set of proprietary factory functions that I need within perfdhcp. I test only those functions. What we want in src/lib/dhcp should be generic not proprietary to perfdhcp.

TestControlTest?.Options6: How does the code for requestes_options work? It uses matchRequestedOptions() that compares the requests on byte by byte basis. DHCPv6 options are coded on 2 bytes. matchRequestdOptions6() is needed.

Ooops! I missed the fact that they are 2 bytes long for DHCPv6.

matchRequestedOptions(): break in the loop should be removed. This will detect option code duplicates.

Ok. Removed.

TestControlTest?.Options6: TODO should look like this / @todo to be picked up by Doxygen TODO list generator.

Fixed.

TestControlTest?.PacketTemplates?: The test should verify that the templates were read correctly.

Test now uses random binary data to create the template files in flight. Then it initializes CommandOptions? with those template files and compares that data read from the files is equal to what has been written to files.

TestControlTest?.RateControl?: Checks for xchgs_num should be a bit more thorough. This is very broad subject, so I proposed to add TODO and don't spend too much time on it now.

I added todo cause it might be quite complex to create more thorough test.

Ok. That's it. End of the review.

Thank you for the effort. I will try to create smaller tickets from now on. I can see that you haven't omitted a single line of code. Good review.

comment:14 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

comment:15 in reply to: ↑ 8 Changed 7 years ago by tomek

Replying to marcin:

Replying to tomek:

getLocalAddress()
How is the code for broadcast is going to work on a device with multiple interfaces? 255.255.255.255 is a local network broadcast address. If your host is connected to 2 networks, it is valid on both. There should be a way to specify which interface is going to be used. I'm not sure how to do it with boost (and IPv4), but there is a field in6_scope_id that can be used for scope in IPv6. Perhaps something similar is available for IPv4. If it isn't, we should at least mention it in the docs. (od add @todo note somewhere).

This comment was not responded. I've added appropriate @todo in the code.

openSocket6()
if (addr.toText() != "::1") was moved to the previous line. It should be in a separate line.

Corrected.

It still looked ugly for me. "if" statement was in the same line as previous statement. Furthermore, there was no curly braces after it. I've pushed fixed code.

I implemented multipleSockets test that combines testing of getSockets() and closeSockets(). It will basically try to open more than one socket, check if list of sockets is matching expected socket descriptors. It will also try to use sockets (expecting success), close them with closeSockets() and try to use them again (expecting failure).

In the socketFromRemoteAddress I added new section to test broadcast address like this:

 int socket3  = 0;
    IOAddress bcastAddr("255.255.255.255");
    EXPECT_NO_THROW(
        socket3 = ifacemgr->openSocketFromRemoteAddress(bcastAddr, PORT2);
    );
    EXPECT_GT(socket3, 0);
    close(socket3);

It is all that can be done now, but it does not address the main issue. How do you specify which interface should be used if you have eth0 and eth1? Broadcast is valid on both. You have no control over which interface is going to be used.

That's why I said that it is not easily solvable now. I've added a @todo in the code and consider the issue closed. We will have to get back to it eventually, but it will not be in a near future.

Since SocketsCollection? is now used outside of the Iface class, perhaps SocketsCollectionIter? type should be defined? I don't have strong opinion here, so it may be left as it is now.

I left it untouched for now. Writing SocketCollection::iterator or SocketCollection::const_iterator does not cost a lot comparing to SocketCollectionIter?. Assuming that objects in SocketCollection? are rather read only, clients should use const_iterator. Shouldn't it be SocketCollectionConstIter? defined instead?

Ok. I've added a @todo in the text. That's yet another thing that we will do some time in the future.

Documentation
I see that there's quite a bit of new code in the iface_mgr (added mostly in previous tickets). Some description about how the socket management is done should be added to doc/devel/02-dhcp.dox. In fact, the 20-dhcp.dox can probably be renamed, split and perhaps moved to more suitable directory. Or it can stay in doc/devel - it is up to you.

The key point with the documentation here is that not huge multi-page is needed. It is addressed to C++ developers, who can read the code and understand what each method does separately. It should present "the big picture".

It is planned that we create an additional ticket to update documentation of perfdhcp after code refactoring and integration. I suggest that I implement your suggestion as a part of this new ticket.

Ok.

libdhcp++.cc
I just wanted to say that it is great that the code is being written there. It will be very useful for option definition framework.

What happens if the buffer passed to optionFactory() is not consumed entirely, e.g. there are 20 bytes for an option that conveys a single IPv4 address? There's no way to detect that or report somehow where the parsing ended.

A packet is received and put into some buffer. How is this mechanism going to be used without copying the data to a separate OptionBuffer?, just to call optionFactory() that will create an object that will copy the data again (that copy is unavoidable)?

I did not want to complicate this new factory function. I assumed that the purpose of this function is to find if the appropriate factory function has been registered and pass the call to it.
The necessity to copy the buffers occurs because of Option class constructor that takes the reference to the buffer. This buffer has to be created somewhere. Usually it will be created within the custom factory function or by the client class that calls Option::Factory(). Since it is passed by reference further on, it will not be copy constructed any further until Option constructor that initializes data_ member. In fact we have just one copy operation that we can't really avoid.

My point was that Option class should also take pointers (or iterators) to first and last byte of the buffer, so no copying actually occurs. But this is a generic problem of libdhcp, so fixing it in test control ticket wouldn't be appropriate. Added @todo for this.

Option(const Option& buf) format is much easier to use in creating options and in tests.
Option(const Option::const_iterator begin, const Option::const_iterator end) is much easier to use in receiving options. I believe we will eventually need both constructors.

tests/libdhcp++_unittest.cc
The test for optionFactory() is not thorough enough. Dummy function "return OptionPtr?(1);" would have passed it. Additional checks for returned option being the right type of right length and having the right content is needed.

I added requested checks to the test.

Thanks.

Ok, I consider all comments raised in part 1 addressed. Now, on to part two.

comment:16 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by tomek

Replying to marcin:

CommandOptions::printCommandLine() should also print specified command-line parameters as they are. That may seem redundant, but there is very good reason to print them out. It is easy to reproduce the case, based only on log file. It is very useful to keep a format that can be copy-pasted easily into the next execution.

Good suggestion. Tool now always prints the command line as it was typed in.

No, it doesn't. I've tried to run:

./perfdhcp -4 -r 1000 all

and it didn't print out command-line.

perf_pkt4.cc and perf_pkt6.cc
Methods writeAt() and writeValueAt are not tested.

I added tests for these methods and .... I found bug in writeValueAt. Thanks for this! :-)

:-)

pkt_transform.h
writeValueAt() is intended to be used for integer types only, right? Aren't there a simpler way to do it?

They are integers only. What is the simpler way?

I was thinking about something without the loop.

switch (sizeof(T))
{
    case 1:
    // code for storing a single byte
    case 2:
    // code for storing a word
    case 4:
    // code for storing a double word
    ...
};

It would be faster than a loop. Keep in mind that those methods are used many, many times per second. If you are tired of this ticket, turning this into a @todo is ok for now.

PktList? type - I bow to your superior template handling techniques, sir. How many nested templates are there? 6? :) Seriously speaking, I do not understand that PktList? definition.

I don't like having so many nested templates but in fact boost is about templates and some of them may become quite complex in use. The multi_index_container is documented here:
http://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/index.html

Explaining a magic trick takes away the feel of mystery. ;-)

I also added some more comments in the typedef.

Thanks.

printIntermediateStats(): ostringstream objects don't have to be explicitly initialized to empty strings. The default constructor does that as well, but is faster. ++it in "for" loop doesn't have to be written in a separate line.

Corrected. Why are the default constructors faster here?

Because with ostringstream(""), you create a string object that is passed to ostringstream constructor. When using default constructor, no such object is created. The difference is speed is marginal, though.

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

comment:17 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by tomek

Replying to marcin:

The comment just after protected: It should be moved above protected. The general approach is that the comment appears before the piece of code that is being commented. It is a good point about using friends. I tried to use that approach in one ticket, but I later removed it to follow whatever others do. It might be useful to write a question about it to bind10-dev asking if people are ok with using "friends" approach. As the discussion on bind10-dev may be lengthy, carry on with this ticket. If the discussion reaches a conclusion that fried is the way to go, we will have lots of places to change the code anyway.

Comment moved. That's going to be hard job to change all classes to use "friend" approach. Maybe this could be applied to new classes only.

That is a reasonable approach. Nevertheless, you should ask about the concept on bind10-dev.

The naming convention that I am following is use

  • factoryABC6 for functions that are used to create DHCPv6 options only
  • factoryDEF4 for functions that are used to create DHCPv4 options only
  • factoryGHI for functions that can be used for either DHCPv4 or v6 options.

The function names correspond to this. Although it may seem a little redundant the option name indicates which factory function is for which IP mode (for somebody who does not recognize it from the option name). I insist that we leave it as it is.

Sounds reasonable. Please add a comment about that naming scheme.

Many factory methods throw exceptions if specified buffer is of invalid size. That should be reflected in the description with \throw.

I belive it is. Can you point which one that throws exception is left without \throw?

I thought that factory methods create specialized objects, e.g. factoryIana6 creates instances of Option6IA, not Option classes. Since they are using generic class, \throw are not needed.

initTemplateFiles(): Where is the list of template files specified? It should briefly comment on it, as that info is not passed as parameter.

I am not sure if I understand your point. The list of template files is given from command line. I updated comments to reflect this.

Thanks. I see that the method is now called initPacketTemplates(). I like the new name better.

There are many articles missing in the comments in test_control.h. As I've never learned to handle them correctly, I will not bother to guess when they're needed.

I added one or two articles. I haven't found "many".

Here's an example (a comment to the openSocket() method):

    /// Method opens socket and binds it to local address. Function will
    /// use either interface name, local address or server address
    /// to create a socket, depending on what is available (specified
    /// from the command line). If socket can't be created for any
    /// reason, exception is thrown.

I think it should be:
opens socket => opens a socket
to local address => to the local address
If socket can't => If the socket can't
exception is thrown => an exception is thrown

Anyway, I don't think it is productive to spend much time on it. It may sound a bit rough for native speakers, but they will understand it.

receivePacket4(), receivePacket6(): It looks more natural when received packet is returned by the method, so unless there are specific reasons to do otherwise, I would prefer the prototype to be:

const dhcp::Pkt4Ptr receivePacket4(const TestControlSocket& socket);

The packet is NOT returned here at all. It is the input parameter. The actual packet reception from the wire is done elsewhere. The receivePacketX functions simply do processing on received packet (e.g. update statistics on StatsMgr?, calculate RTT etc.).

Uh oh. So receivePacket4 does not receive a packet? So rename it! processReceivedPacket4 sounds reasonable...

byte2Hex(), vector2Hex(): These are very generic methods. Similar methods exist in src/lib/util/encode/hex.h (and other headers in that directory). If there isn't one already defined, please move them there.

My methods in their shape do not fit to src/lib/util/encode/hex.h or any other file there. In the same time, methods provided there do not provide separators. Trying to move my code there might be tricky and I prefer postponing it for now.

Ok. A simple @todo will do the trick for now.

The check for reaching maximum drops percentage can be skipped, if total drop is 0. A simple if around the whole section should do the trick.

Does it really improve any performance here? I don't want this function to grow any bigger than necessary so I dropped this suggestion.

I'm not sure how many times per second it is called. It was only a suggestion to consider, so it is ok to drop it.

This sounds reasonably. Initially I did not think that there will be so many distinct invocations to StatsMgr?<Pkt4> and StatsMgr?<Pkt6>. In the future we may reconsider this but not as a part of this ticket. I will take a look at this as a part of #1960.

1960 seems to be growing bigger and bigger. Will it be ultimately larger than #1959? Perhaps the first thing to do in 1960 would be to split it into smaller chunks?

template_idx is always zero. Are there any cases when additional templates may be used? If not, please add an appropriate comment.

The reason to define the constant is that everybody knows that 0 is template index. I do another constant when I get the template for request packet. I just avoid using unnamed '0's in the code. I added some comment there.

This should be a const static member of the class then.

There is no check if received ADVERTISE is sane or not. First, the code should check if there is STATUS_CODE option in the message with non-zero status. Then it should check for non-zero STATUS_CODE opion in the IA_NA option. Finally it should check if there's IAADDR option in IA_NA. If you don't want to implement this now, just adding appropriate @TODO will be sufficient for now.

We will need some other ticket for this to guarantee robustness of this code but also in many other places.

Yes. Please create one.

updateSendDue(): Is the code really that fast that it usually send out packeted within microsecond of when it was supposed to be sent? I would add a little delta when comparing now and send_due_ to avoid cases when things are happening at the boundary between n-th and n-th microsecond. Some time later, this could be made a configurable parameter ("allowed send delay" or "send delay tolerance"), but it is ok to add @todo for now (if you think it is a good idea to implement such a parameter).

In fact, I have checked that it is often that we exceed due time and latesend counter is increased. First I would like to implement milisecond timeout in ifaceMgr as it will change a lot the timings in main loop. After that I will take a look how it affects the section you're poiting at.

Ok. That seems like a worthy candidate for a separate ticket. Please create one with a pointer to this discussion.

I have removed many comments that you agreed with or just marked as one. It will make reading this at least a bit easier.

comment:18 in reply to: ↑ 13 Changed 7 years ago by tomek

Replying to marcin:

CommandOptionsHelper? seems like a useful class that could be shared between other components. dhcp4 and dhcp6 come to mind, but I'm sure there are others. You should investigate, if it is feasible to move it to src/lib/testutils.

I will investigate this in the course of #1960. I just don't want to complicate things more than they are now.

Partially agree. That should not be handled in 1959, but stuffing it into 1960 will just make another bloated 2-weeks/3 days review ticket. Please create a separate ticket for it. It is ok to have small (like half a day) ticket.

We do have plans to implement interface detection on other OSes, but such a ticket is not currently planned. Do you think it would be useful to implement it soon? There is a getifaddrs() interface that seems to be supported on Linux, all BSDs and surprisingly even Solaris 11.

I think it would be very useful so as we can run tests on multiple OSes.

I've just created ticket #2246 for it. If you think it is useful to have it sooner rather than later, you may suggest that it should be included in the next sprint.

I don't like the fact that if no interfaces are detected that the test does nothing. In my opinion it should fail in such case.

I understand your point but I would rather want to have reliable interface detection on all supported OSes. The interface detection should be checked elsewhere (IfaceMgr?). Here I rely on this fact but I keep this safety valve as long as I am not sure if it is 100% reliable and well tested.

Fair enough.

getLocalLoopback(): Loopback interfaces should have IfaceMgr::Iface::flag_loopback_ set to true.

Nice! I now use this although I don't like the fact that this field is public.

I was too lazy/busy to implement getters for it.

If I understand the method correctly, in its current it will never return anything above 2. The first iteration will reduce the value to something in 0..255 range. The second iteration will set unequal_pos to 2 and break the loop. For that reason it doesn't work for 65537 and above (returns 2 instead of 3).

The code for this method is surprisingly convoluted. If I understand it correctly, this method should return number of bytes that have to be different to accommodate specified number of clients. So what you really want is:

log256(clients_num - 1);

Here are 2 proposals to do this:

    if (clients_num < 2) // degenerated case. There's nothing to randomize
        return 0;

    return 1 + log(clients_num - 1)/log(256);

It is very short, but has the disadvantage of using floating point arithmetic. Another code that is similar to the code under review would be:

    if (!clients_num)
        return 0;
    clients_num--;

    int cnt = 0;
    while (clients_num) {
        clients_num >>= 8;
        ++cnt;
    }

    return cnt;

The advantage here is not using floating point calculations. As this is a test code, I personally prefer simplicity over performance to avoid possible bugs.

Since you approved both options I picked the second one. I don't like floating points. :-)

Ok.

On a related note, how does this method correlate to what is happening in generateDuid()? If you have 256 clients, those clients will have unique DUID only if the generateMacAddress() manages to allocate 256 unique numbers.

I would like to see a test that generates 256 DUIDs and verifies that they are indeed unique. They should differ only on a single byte, right?

I used random() function to generate "unique" numbers. The point is that they might not be unique in the short period of time because repetitions are possible. We discussed this on jabber and I think what strikes me here is that "if somebody sets he wants to simulate 100 clients and run 100 iterations he might not get 100 leases and he will be unhappy trying to debug why it is the case". My understanding was that if you run the test for a long period of time (BTW, this is how performance should be measured) the number of leases will reach 100 or will be close enough. However, after our discussion I think it is good point to guarantee the unique DUIDs and MACs within number of iterations equal to number of simulated clients. I do it now with sequential number generators.

Ok, great.

TestControlTest?.Options4,6: Those tests are generic to all options. Most of the test code is generic, only the call to registerOptionFactories() is specific to TestControl?. You should consider splitting those tests and moving generic parts to src/lib/dhcp/ somewhere.

I don't agree. I have a set of proprietary factory functions that I need within perfdhcp. I test only those functions. What we want in src/lib/dhcp should be generic not proprietary to perfdhcp.

Ok, fair enough. Nevertheless, parts of the existing code will be reused option definition work will progress to the stage that factory method will be used in src/lib/dhcp as well.

TestControlTest?.Options6: How does the code for requestes_options work? It uses matchRequestedOptions() that compares the requests on byte by byte basis. DHCPv6 options are coded on 2 bytes. matchRequestdOptions6() is needed.

Ooops! I missed the fact that they are 2 bytes long for DHCPv6.

matchRequestedOptions6() look ok.

Ok. That deals with the response to 4th part of the review. I'm afraid during the process of answering them, I've found couple new things. I will post them in a separate comment. Don't worry, they are very minor.

comment:19 follow-up: Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

I've found couple of new minor issues:

command_options.h

Some lines are too long. In DHCP4 there was a hard limit of 80 columns. With BIND10 we decided on a soft limit around 100 characters. Some lines are longer than that. There was a discussion some time ago on bind10-dev list, but I don't think anyone bothered to update coding guidelines.

command_options.cc

version(): I think perfdhcp should follow the same versioning scheme as rest of BIND10. Please include config.h and return VERSION defined in it. It will be much easier to maintain. Otherwise you risk having to perform your own releases. Believe me - you want to avoid that. :-)

usage() method should print out a couple (3-4) example usages. Understanding which parameters are mandatory and which are optional is not trivial.

test_control.cc
factoryOptionRequestOption6(): the buffer is defined wrong. The order is reversed. It should be in network byte order (0, 23, 0, 24, not 23, 0, 24, 0). The current buffer requests options 5888 and 6144. The same mistake is repeated in TestControlTest?.Options6.

Ok, that's it. I have no further comments.

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

Replying to tomek:

Replying to marcin:

CommandOptions::printCommandLine() should also print specified command-line parameters as they are. That may seem redundant, but there is very good reason to print them out. It is easy to reproduce the case, based only on log file. It is very useful to keep a format that can be copy-pasted easily into the next execution.

Good suggestion. Tool now always prints the command line as it was typed in.

No, it doesn't. I've tried to run:

./perfdhcp -4 -r 1000 all

and it didn't print out command-line.

Try ./perfdhcp2 -4 -r 1000 all. That one will work :-)

perf_pkt4.cc and perf_pkt6.cc
Methods writeAt() and writeValueAt are not tested.

I added tests for these methods and .... I found bug in writeValueAt. Thanks for this! :-)

:-)

pkt_transform.h
writeValueAt() is intended to be used for integer types only, right? Aren't there a simpler way to do it?

They are integers only. What is the simpler way?

I was thinking about something without the loop.

switch (sizeof(T))
{
    case 1:
    // code for storing a single byte
    case 2:
    // code for storing a word
    case 4:
    // code for storing a double word
    ...
};

It would be faster than a loop. Keep in mind that those methods are used many, many times per second. If you are tired of this ticket, turning this into a @todo is ok for now.

I am still not confident about the benefit from changing the current code to switch statement but I added a @todo for this.

PktList? type - I bow to your superior template handling techniques, sir. How many nested templates are there? 6? :) Seriously speaking, I do not understand that PktList? definition.

I don't like having so many nested templates but in fact boost is about templates and some of them may become quite complex in use. The multi_index_container is documented here:
http://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/index.html

Explaining a magic trick takes away the feel of mystery. ;-)

I also added some more comments in the typedef.

Thanks.

printIntermediateStats(): ostringstream objects don't have to be explicitly initialized to empty strings. The default constructor does that as well, but is faster. ++it in "for" loop doesn't have to be written in a separate line.

Corrected. Why are the default constructors faster here?

Because with ostringstream(""), you create a string object that is passed to ostringstream constructor. When using default constructor, no such object is created. The difference is speed is marginal, though.

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

Replying to tomek:

Replying to marcin:

The comment just after protected: It should be moved above protected. The general approach is that the comment appears before the piece of code that is being commented. It is a good point about using friends. I tried to use that approach in one ticket, but I later removed it to follow whatever others do. It might be useful to write a question about it to bind10-dev asking if people are ok with using "friends" approach. As the discussion on bind10-dev may be lengthy, carry on with this ticket. If the discussion reaches a conclusion that fried is the way to go, we will have lots of places to change the code anyway.

Comment moved. That's going to be hard job to change all classes to use "friend" approach. Maybe this could be applied to new classes only.

That is a reasonable approach. Nevertheless, you should ask about the concept on bind10-dev.

The naming convention that I am following is use

  • factoryABC6 for functions that are used to create DHCPv6 options only
  • factoryDEF4 for functions that are used to create DHCPv4 options only
  • factoryGHI for functions that can be used for either DHCPv4 or v6 options.

The function names correspond to this. Although it may seem a little redundant the option name indicates which factory function is for which IP mode (for somebody who does not recognize it from the option name). I insist that we leave it as it is.

Sounds reasonable. Please add a comment about that naming scheme.

Ok. I added some comment in class description.


Many factory methods throw exceptions if specified buffer is of invalid size. That should be reflected in the description with \throw.

I belive it is. Can you point which one that throws exception is left without \throw?

I thought that factory methods create specialized objects, e.g. factoryIana6 creates instances of Option6IA, not Option classes. Since they are using generic class, \throw are not needed.

initTemplateFiles(): Where is the list of template files specified? It should briefly comment on it, as that info is not passed as parameter.

I am not sure if I understand your point. The list of template files is given from command line. I updated comments to reflect this.

Thanks. I see that the method is now called initPacketTemplates(). I like the new name better.

There are many articles missing in the comments in test_control.h. As I've never learned to handle them correctly, I will not bother to guess when they're needed.

I added one or two articles. I haven't found "many".

Here's an example (a comment to the openSocket() method):

    /// Method opens socket and binds it to local address. Function will
    /// use either interface name, local address or server address
    /// to create a socket, depending on what is available (specified
    /// from the command line). If socket can't be created for any
    /// reason, exception is thrown.

I think it should be:
opens socket => opens a socket
to local address => to the local address
If socket can't => If the socket can't
exception is thrown => an exception is thrown

Anyway, I don't think it is productive to spend much time on it. It may sound a bit rough for native speakers, but they will understand it.

receivePacket4(), receivePacket6(): It looks more natural when received packet is returned by the method, so unless there are specific reasons to do otherwise, I would prefer the prototype to be:

const dhcp::Pkt4Ptr receivePacket4(const TestControlSocket& socket);

The packet is NOT returned here at all. It is the input parameter. The actual packet reception from the wire is done elsewhere. The receivePacketX functions simply do processing on received packet (e.g. update statistics on StatsMgr?, calculate RTT etc.).

Uh oh. So receivePacket4 does not receive a packet? So rename it! processReceivedPacket4 sounds reasonable...

I renamed per your suggestion.

byte2Hex(), vector2Hex(): These are very generic methods. Similar methods exist in src/lib/util/encode/hex.h (and other headers in that directory). If there isn't one already defined, please move them there.

My methods in their shape do not fit to src/lib/util/encode/hex.h or any other file there. In the same time, methods provided there do not provide separators. Trying to move my code there might be tricky and I prefer postponing it for now.

Ok. A simple @todo will do the trick for now.

Added @todo.

The check for reaching maximum drops percentage can be skipped, if total drop is 0. A simple if around the whole section should do the trick.

Does it really improve any performance here? I don't want this function to grow any bigger than necessary so I dropped this suggestion.

I'm not sure how many times per second it is called. It was only a suggestion to consider, so it is ok to drop it.

Dropped.

This sounds reasonably. Initially I did not think that there will be so many distinct invocations to StatsMgr?<Pkt4> and StatsMgr?<Pkt6>. In the future we may reconsider this but not as a part of this ticket. I will take a look at this as a part of #1960.

1960 seems to be growing bigger and bigger. Will it be ultimately larger than #1959? Perhaps the first thing to do in 1960 would be to split it into smaller chunks?

I don't think it is going to be larger. I think you may be right. I will create another ticket for this.

template_idx is always zero. Are there any cases when additional templates may be used? If not, please add an appropriate comment.

The reason to define the constant is that everybody knows that 0 is template index. I do another constant when I get the template for request packet. I just avoid using unnamed '0's in the code. I added some comment there.

This should be a const static member of the class then.

For now I removed confusing const and I use index directly with comment. In the further implementation of perfdhcp we will be adding support for other message types (e.g. RENEW). This will be the opportunity to re-think the mapping between message type and index of vector that holds template buffers and many other "indexed" command options.


There is no check if received ADVERTISE is sane or not. First, the code should check if there is STATUS_CODE option in the message with non-zero status. Then it should check for non-zero STATUS_CODE opion in the IA_NA option. Finally it should check if there's IAADDR option in IA_NA. If you don't want to implement this now, just adding appropriate @TODO will be sufficient for now.

We will need some other ticket for this to guarantee robustness of this code but also in many other places.

Yes. Please create one.

Created #2250.

updateSendDue(): Is the code really that fast that it usually send out packeted within microsecond of when it was supposed to be sent? I would add a little delta when comparing now and send_due_ to avoid cases when things are happening at the boundary between n-th and n-th microsecond. Some time later, this could be made a configurable parameter ("allowed send delay" or "send delay tolerance"), but it is ok to add @todo for now (if you think it is a good idea to implement such a parameter).

In fact, I have checked that it is often that we exceed due time and latesend counter is increased. First I would like to implement milisecond timeout in ifaceMgr as it will change a lot the timings in main loop. After that I will take a look how it affects the section you're poiting at.

Ok. That seems like a worthy candidate for a separate ticket. Please create one with a pointer to this discussion.

Created ticket #2251.

I have removed many comments that you agreed with or just marked as one. It will make reading this at least a bit easier.

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

Replying to tomek:

I've found couple of new minor issues:

command_options.h

Some lines are too long. In DHCP4 there was a hard limit of 80 columns. With BIND10 we decided on a soft limit around 100 characters. Some lines are longer than that. There was a discussion some time ago on bind10-dev list, but I don't think anyone bothered to update coding guidelines.

I cleaned this up in #1960.

command_options.cc

version(): I think perfdhcp should follow the same versioning scheme as rest of BIND10. Please include config.h and return VERSION defined in it. It will be much easier to maintain. Otherwise you risk having to perform your own releases. Believe me - you want to avoid that. :-)

I want to avoid it. This is why I now return VERSION. Thanks.

usage() method should print out a couple (3-4) example usages. Understanding which parameters are mandatory and which are optional is not trivial.

This is not good candidate for this ticket. It is documentation kind of thing. I will include this in one of the tickets about documentation.

test_control.cc
factoryOptionRequestOption6(): the buffer is defined wrong. The order is reversed. It should be in network byte order (0, 23, 0, 24, not 23, 0, 24, 0). The current buffer requests options 5888 and 6144. The same mistake is repeated in TestControlTest?.Options6.

Fixed. Thanks.

Ok, that's it. I have no further comments.

Thanks for review.

comment:23 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

comment:24 Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

I just went over my comments and your responses. It seems that you haven't responded to the idea of making CommandOptionsHelper? a generic class in comment 18. Please do it as part of 1960 work or as a separate ticket (whichever you prefer).

With this last thing dealt with, I declare this ticket fully reviewed!

Please merge.

It's the end. Yay!

comment:25 Changed 7 years ago by marcin

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

The trac1959 has been merged to master. Yay!

Note: See TracTickets for help on using tickets.