Opened 8 years ago

Closed 7 years ago

#1954 closed enhancement (fixed)

Implement perfdhcp command parser

Reported by: stephen Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20120528
Component: perfdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 16 Add Hours to Ticket: 6
Total Hours: 48 Internal?: no

Description

Extract the command parsing of perfdhcp into a separate class.

Subtickets

Change History (13)

comment:1 Changed 8 years ago by marcin

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

Doing some work around this ticket will be helpful to get familiar with BIND10 build environment (ticket: 1950) as it will include unit-tests addition, new class file addition etc.

comment:2 Changed 8 years ago by marcin

  • Add Hours to Ticket changed from 0 to 2

comment:3 Changed 8 years ago by stephen

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

comment:4 Changed 8 years ago by marcin

  • Add Hours to Ticket changed from 2 to 36
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

Added CommandOptions? class along with unit tests and refactored the C code to C++ where possible. I also made a copy of perfdhcp.c to perfdhcp.cc to make it compile with C++ compiler and link properly with new C++ files.
Verifed that new code compiles on Ubuntu (clang/gcc), MacOS (clang), FreeBSD 8.2 (gcc), Solaris (Sunstudio).

comment:5 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 Changed 8 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit d81f573764fe2df0006527145e439893872e6c69

tests/tools/perfdhcp/command_options.h
Should have a comment explaining what ExchangeMode is.

DORR_SARR should be DORA_SARR (final packet in an IPv4 exchange is an Ack)

Do we need force_reset? The command should only be parsed once in the the main perfdhcp code so it is not needed. And during testing, resetting the state of the variables on each parse seems to be the logical way to use the class.

getSidOffset() (etc). How often are these functions this used in the code? If just once, it may be clearer to expand the names (e.g. getServerIdOffset()).

Default constructor: can this be private (a more restrictive scope)?

check(): A bit more documentation is needed - although the functionality can be deduced by reading the description of the exception, just something like "Convenience function that throws an InvalidParameter exception if the conditional argument is false" is clearer.

validate(): typo in InvalidParameter

decodeBase(): An explanation of what the "base" is would be useful. (Likewise for decodeMac and decodeDuid. A sentence here saves a couple of minutes finding the help documentation and locating the information for the "-b" flag.)

tests/tools/perfdhcp/command_options.cc
The single instance of CommandOptions can be more simply be created by:

CommandOptions&
CommandOptions::instance() {
    static CommandOptions options();
    return (options);
}

(A static variable within a function is initialized the first time the function is called. Thereafter the initialization is skipped.)

reset(): Add a comment explaining what "mac" and "lt" are (and why they don't default to zeroes).

reset(): calling vector::resize(0) to empty is vector is correct, although I get the impression that calling clear() is more usual.

initialize(): the use of lexical_cast here does not take account of the fact that the argument could be of the incorrect type and that lexical_cast would throw an error. Given the number of command switches that require a positive integer, a lot of the processing could be extracted into a separate function, e.g.

int
positiveInteger(const char* errmsg) {
   try {
      int value = boost::lexical_cast<int>(optarg);
      check(value <= 0, errmsg);
      return (value);
   } catch (boost::bad_lexical_cast&) {
      isc_throw(InvalidParameter, errmsg);
   }
}

(A similar function could be created to check for non-negative integers.)

initialize(): Useful to document all the local variables with an inline comment. Also, unless there is a good reason, I suggest avoiding single-character or two-character names - it is less clear for a reader.

initialize(): Clearer to explicitly declare "of" on a line by itself. (Declaring variables one per line is preferred as it encourages the addition of an in-line comment describing the variable.)

initialize(): The "switch" statement should be indented with respect to the "while".

initialize(): Should "-h" cause parsing to terminate?

initialize(): "r" is declared as "long long" but is then cast to unit32_t. This is not safe if "long long" is 64 bits. Why not directly lexical_cast optarg to uint32_t?

initialize(): More descriptive names in the calculations instead of "m, b and s" would be helpful. Also "m" can be declared on the line in which it is set instead of as the second variable declaration in the preceding line.

initialize(): Even single-line "if" clauses must be enclosed in braces (in the checking of ipversion_ equal to zero, and in the following "if" checks.)

initialize(): when there are only two boolean checks in an "if" check, it is probably clearer to put them both on the same line if they fit (block of code prefixed "Decode special cases").

initialize(): check the boolean variables. broadcast_ and rapid_commit_ (amongst others) are boolean, so should be set to "true", not to "1".

decodeBase(): What if the "-b" argument is unknown? (In other words, should there be an "else" clause?)

decodeBase(): if the base is a MAC address, the code expects "mac=" to be lower-case. Yet the error message in decodeMac indicates that "MAC=" should be uppercase.

decodeMac(): "found+1" should be "found + 1".

decodeMac(): Although the compiler should take care of it, suggest that the error message be declared as a "static const char*" variable and that the variable be passed to the check() call.

usage(): suggest that we omit the "\f" (form-feed) character in the output.

version(): just a remark that this will need to be changed once we've sorted out how we are going to maintain a version number.

tests/tools/perfdhcp/tests/command_options_unittest.cc
Test Base: if the vector::operator==() fails on FreeBSD, does std::equal() work? In other words, does

EXPECT_TRUE(std::equal(v1.begin(), v1.end(), mac));

... work?

Test Base: not too important for an int, but in general "++obj" is preferred to "obj++" as it eliminates the need to hold a temporary object.

Test Base: should check for invalid MAC and DUID values.

Test DropTime: is this test correctly named? It checks the LostTime variables. (Or is the LostTime variable incorrectly named?)

Tests ReportDelay/RandomRange/LocalPort/Preload/Seed: should also check for invalid values of the argument.

comment:7 Changed 8 years ago by marcin

  • Add Hours to Ticket changed from 36 to 6
  • Estimated Difficulty changed from 0 to 16
  • Owner changed from marcin to stephen

Thanks for thorough code review. I updated code accordingly.

A couple of answers:
version(): just a remark that this will need to be changed once we've sorted out how we are
going to maintain a version number.

Agree

Test Base: if the vector::operator==() fails on FreeBSD, does std::equal() work? In other words, does
EXPECT_TRUE(std::equal(v1.begin(), v1.end(), mac));
... work?

Yes, I corrected this in the test.

Tests ReportDelay/RandomRange/LocalPort/Preload/Seed?: should also check for invalid values of the argument

Number of new negative test cases added

comment:8 Changed 8 years ago by marcin

  • Total Hours changed from 0 to 42

comment:9 Changed 8 years ago by stephen

  • Owner changed from stephen to marcin

Review commit c65a54347a7bceee54531a6a81209d1ec6ceb82b

test/tools/perfdhcp/command_options.h
There are two "private" declarations (the second coming from changing "protected" to "private"). The second can be removed.

The description of the default constructor should start "Private constructor..." (not "Protected constructor...").

In the description of the check() method, there is no need to prefix InvalidParameter with an exclamation mark. (That is only needed when documenting in the wiki.)

positiveInteger() and related methods can be declared "const", as they don't change anything in the class.

Abbreviations are best avoided (where reasonable) in the documentation as it makes it more like English and easier to read, e.g. "Casts command line arg to positive int" is better as "Casts command line argument to positive int". (Note: int - and not "integer" - being used as this is the name of a C++ type.)

initRandomRange(): The exception is thrown if the -R value is wrong - but what constitutes wrong? It would be useful for the description to explain this.

decodeBase(): Several points:

  • Explanation of what the "base" is would be useful (e.g. "Method decodes the argument of the '-b' switch, which specifies a base value used to generate unique mac or duid values in packets sent to the system under test.")
  • (trivial) the description is 55 characters wide on the first line and 81 on the second - it is easier to read if the lines are approximately the same length. (No maximum line length is given in the guidelines, but usually blocks of comments reach to column 80 at most.) The three lines after the two examples can also extend to column 80.
  • "Currently is supports the following command line options, e.g." is perhaps better written as "The following forms of switch arguments are supported:"
  • The two lines should be prefixed by "- " to generate a list (see http://www.stack.nl/~dimitri/doxygen/markdown.html).
  • Typo in "respectively"

decodeDuid(): Suggest use of the definite article where needed (i.e. "The function will..." rather than "Function will"). Also, the example in the description of the parameter "base" is a MAC example, not a DUID example.

convertHexString(): Suggest title be "Convert two-digit hexadecimal string to a byte". Suggest also using a more descriptive name for the parameter (e.g. "hex_text" rather than "s"). Finally, this can be a "const" method as it does not modify anything in the class.

Should drop_time_set_ and max_drop_set_ be bool instead of int?

drop_time_set_ comments refers to losttime, which was renamed in the last set of changes.

test/tools/perfdhcp/command_options.cc
reset(): Comments should follow normal rules of English and start with a capital letter.

parse(): Resets the optind/opterr variables, but should it not also call reset()?

initialize(): The coding guidelines state that lowercaseUppercase is the format reserved for method names. Local variables should be all lower-case, possibly separated by underscores (e.g. lower_case).

initialize(): Would prefer the in-line comments to start with a capital letter (and the same really goes for the inline variables in the header file). Also typo in "holiding".

initialize() - (parsing "-D"): the possibility of lexical_cast() throwing an exception is ignored.

initialize() - (parsing "-D"): on inspection, it appears that the code is assuming that the format of the switch is "-D %<number>" (as substr() with a single argument is taking the contents of the string from the position given by the argument to the end of the string). (It also appears that there is no unit test for the "-D n%" form, else this would have been caught.)

initialize() - (parsing "-D"): the structure is currently:

if (...) {
    :
    break;
}
:
break;

I think clearer is:

if (...) {
   :
} else {
   :
}
break;

(i.e. one exit point).

initialize() - (parsing "-L"): not need for a space following the left parenthesis in "static_cast<int>( ".

initialize() - (parsing "-T"): An if/else (with a test against the value 2) seems more logical than a switch statement here.

initialize() - (parsing "-O"): Can't positiveInteger() be called instead of using a try/catch block?

initialize() - (parsing "-O" and "-X"): is the logic correct here? If I specify multiple "-O" or "-X" values on the command line, it appears that all but the last one or two are ignored.

initialize() - ("default" in the switch statement): the error message appears to be short enough for the call to isc_throw() to be on one line.

initialize() - (general): I think it would be easier to follow if the switches appeared in alphabetical order.

initialize(): Comment preceding the checking of "ipversion_ == 0" does not make sense. Something like "If the IP version was not specified in the command line, assume IPv4." is better.

initialize(): Not clear why we are "tuning up template file lists". Why is the first value in the xid/rnd offset array being duplicated?

initialize(): In the "Get server argument" block, a comment that FF02::1:2 and FF02::1:3 are the RFC3315-defined All_DHCP_Relay_Agents_and_Servers and All_DHCP_Servers addresses would be helpful. (And for completeness, add a comment stating that 255.255.255.255 is the IPv4 broadcast address.)

initRandomRange(): "errmsg" does not need to be "std::string" - it could be declared as "const char*" (which avoids the need to call c_str() before using it).

initRandomRange(): why is "randomRange" (which should be random_range - see note on variable names) declared as "long long"? The "opt" argument is cast to "long long", then that value is cast to uint32_t. Why not cast to uint32_t directly?

initRandomRange(): appears to be some odd spacing before the "=" sign when setting maxTargetRand and maxRandom (which should be renamed max_target_rand and max_random).

initRandomRange(): not quite clear what the comment including "range is already multiple of number of clients range" means.

initRandomRange(): no space between the template parameter and the left parenthesis in the cast of restrictedRandom.

usage(): The documentation for "-w" seems cryptic. presumably the argument is a program called through use of "system()" and passed "start" or "stop"?.

usage(): The documentation for diagnostics should say that unrecognised key letters are ignored.

test/tools/perfdhcp/tests/command_options_unittest.cc
CommandOptionsTest::process(): would prefer a more descriptive name to the argument than "s".

IpVersion: need a test that an exception is thrown if both -4 and -6 are given together.

IpVersion: the comment "negative test cases" is not strictly correct - these checks are checking that invalid combinations of flags are caught.

RandomRange: according to usage(), the "-R" switch specifies the number of different clients. It does not indicate that anything is random. Is the use of the word "random" justified?

Base: need to check invalid values passed as arguments to "mac=" and "duid=" are caught.

DropTime: question: reading usage() suggests that DropTime should be a double value. Yet DropTime is stored as as array of two doubles: why?

TimeOffset: -E only appears to be valid with -T, so checking for an invalid value of -3 should be done with -T present in the command line.

ExchangeMode: not clear why the negative test cases should fail (e.g. the one with "-E 3" should preumably fail because there is no -T present.)

LocalPort: should check that a port value above 65535 should cause a failure.

There appear to be no tests for: -a, -D, -n, -p.

comment:10 Changed 8 years ago by marcin

Most of the suggestions implemented and pushed with commit number:

3f32b68d6f2d0f02af6cebac046a604751ad4dcc

Q: initRandomRange(): "errmsg" does not need to be "std::string" - it could be declared as "const char*" (which avoids the need to call c_str() before using it).

A: It could be, but I want to be consistent with other functions where I declare string arguments as const std::string&

Q: initRandomRange(): why is "randomRange" (which should be random_range - see note on variable names) declared as "long long"? The "opt" argument is cast to "long long", then that value is cast to uint32_t. Why not cast to uint32_t directly?

A: I want to verify that random range is not negative. If I cast to uint32_t I loose information that user specified negative value and I approve it. Lexical_cast does not throw exception if I cast negative value (string) to unsigned. Or it does?

Q: IpVersion?: need a test that an exception is thrown if both -4 and -6 are given together.

A: There is such a test in TEST_F(CommandOptionsTest?, IpVersion?)

Q: IpVersion?: the comment "negative test cases" - these checks are checking that invalid combinations of flags are caught.

A: By negative test cases I exactly mean invalid combinations of flags are caught. Isn't it what negative tests are about?

Q: RandomRange?: according to usage(), the "-R" switch specifies the number of different clients. It does not indicate that anything is random. Is the use of the word "random" justified?

A: Random range value is a range of values that we use to replace last four octets of default DUID value in outgoing packages to simulate different clients. Eventually, different clients have different random values so this value maps to number of clients. In order to avoid confusion I renamed some getter methods, members etc to reflect that it is number of clients. In the future maybe we could change this in documentation too?

Q: Base: need to check invalid values passed as arguments to "mac=" and "duid=" are caught.

A: There are test cases like this in TEST_F(CommandOptionsTest?, Base)

Q: DropTime?: question: reading usage() suggests that DropTime? should be a double value. Yet DropTime? is stored as as array of two doubles: why?

A: Comments in the code updated

comment:11 Changed 8 years ago by marcin

  • Owner changed from marcin to stephen
  • Total Hours changed from 42 to 48

comment:12 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 3f32b68d6f2d0f02af6cebac046a604751ad4dcc

I committed a couple of minor changes (use "git pull" to retrieve them): I removed a redundant initialization in CommandOptions::reset() (a line was duplicated) and corrected the method header of initClientsNum() (still referred to the "random" variables).

If you are happy with these changes, please merge - the rest of your changes are OK.

I don't think this needs an entry in the ChangeLog: I suggest that we wait until the refactoring has been completed and do one then.

comment:13 Changed 7 years ago by marcin

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

Code has been merged to master.

Note: See TracTickets for help on using tickets.