BIND 10 #2272: Improve the perfdhcp command_options_helper code.

[BIND 10 Development]

#2272: Improve the perfdhcp command_options_helper code.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  marcin                             |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  defect                             |  DHCP-20121004
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  Unclassified                       |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 3148cf2b018b222e263894b80cd7e92b20c86ceb
 >
 > '''configure.ac'''[[BR]]
 > Missing semicolon at the end of the "optreset=1" statement in the
 AC_TRY_LINK code for checking optreset. (Although according to my
 config.log, configure added one anyway.)

 Corrected.

 >
 >
 > '''tests/tools/perfdhcp/tests/command_options_helper.h'''[[BR]]
 > The "results" array allocated is too long: the number of elements in the
 array should not be multiplied by "sizeof(char*)".

 ooops! That looks like typical mistake I make when I replace malloc with
 new.

 >
 > The !CommandOptionsHelper class is only used in unit tests using Gtest.
 With this in mind, when allocating memory with "new", rather than using
 "new (std::nothrow)" then an assert() to check that the returned result is
 non-null, why not write a standard "new" within an EXPECT_NO_THROW?

 That would work as well but my intention was to separate errors that come
 purely from invocation of CommandOptions::parse() from errors that come
 from helper methods, which are only to prepare the input to parse(). If I
 follow your suggestion I would get the error similar to this when new
 fails:

 {{{
 test_control_unittest.cc:633: Failure
 Expected: processCmdLine("perfdhcp -l 127.0.0.1 all") doesn't throw an
 exception.
   Actual: it throws.
 }}}

 which tells me nothing where the error comes from and since failure to
 allocate memory with new would be very rare, my intuition would tell me
 that it is exception produced by CommandOptions::parse (which would not be
 true). Apart from this all tests will be run and each will end be
 interrupted by ASSERT_NO_THROW because it is unlikely that if allocation
 issue occurs for the first time it will not occur later.

 If I leave as it is now, I get:
 {{{
 lt-run_unittests: command_options_helper.h:119: static char**
 isc::perfdhcp::CommandOptionsHelper::tokenizeString(const string&, int&):
 Assertion `results == __null' failed.
 }}}

 and I immediately know that it is the error in helper function, not in the
 tested class. In addition unit testing breaks (and I belive it should) as
 this is a fatal error.

 >
 >
 > '''tests/tools/perfdhcp/tests/command_options_helper.cc'''[[BR]]
 > > There are at least two ways to reset the state of the getopt() to
 prevent it from trying to access freed buffers. On Linux: the optind must
 be set to 0. This however does not work on BSD and MacOS where there is an
 additional variable declared: optreset. On those systems both optreset and
 optind must be set to 1.
 > This seems to imply that both optind and optreset must be set to 1 on
 BSD systems only (i.e. with HAVE_OPTRESET set to 1).  However the code
 only sets optreset according to this preprocessor symbol; the setting of
 optind is determined by the macro !__GLIBC!__.

 I am not sure if I understand the comment.
 If HAVE_OPTRESET is defined it means that optreset variable exists and I
 can set it. If it is undefined I can't set it. If it exists the only value
 I should set it to is 1 (there is no other value I should be setting it
 to). I don't set optind in HAVE_OPTRESET clause because I am setting it if
 GLIBC is undefined. If I am working on BSD the GLIBC will be undefined
 which means that optind will be set to 1, then optreset set to 1 which is
 what I want. If GLIBC is there the optind must be set to 0 and then there
 is no HAVE_OPTRESET because GLIBC does not need optreset (because it
 resets through optind=0). I think, this code is going to work in each
 case.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2272#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development