BIND 10 #3059: Create initial UpdateMgr class

[BIND 10 Development]

#3059: Create initial UpdateMgr class
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130731
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

 Replying to [comment:5 marcin]:
 > '''src/bin/d2/d2_cfg_mgr.h'''

 True. Done.

 > '''src/bin/d2/d2_cfg_mgr.cc'''
 > reverseIpAddress: Neat pick: istead of using !''getFamily()!'' you could
 use !''IOAddress
 > ::isV4()!'' which returns boolean function which is shorter.
 > The same applies to reverseV4Address and reverseV6Address.

 Done.

 >
 > Also please check the spaces between the exception text and double colon
 in those functions. E.g.
 > {{{
 >         isc_throw(D2CfgError, "D2CfgMgr cannot reverse address :"
 >                               << address << " : " << ex.what());
 >
 > }}}
 > There is a space after !''address!'' but no space before the actual
 address value.
 >

 Got it.

 > Could you use the static_cast instead of C-style cast in
 reverseV4Address and reverseV6Address?

 Sorry, that is just an old-habit that dies hard.  Fixed.


 >
 > reverseV4Address: Could you use vector's reverse iterator to inverse the
 digits order.
 >

 Yes, but I wanted to see if you were paying attention. Done.

 > '''src/bin/d2/d2_config.h'''
 > The idea/use of wildcard domain should be better explained.

 Added a bit more explanation, hopefully this is sufficient.
 >
 > '''src/bin/d2/d2_config.cc'''
 > matchDomain: Why do you have to reset the domain pointer on the function
 entry? I suggest that the pointer provided by the caller is not modified
 if there is no match. I would understand that the pointer is reset if
 there was no boolean value return which signals that the match was found.
 You could choose to signal no match with NULL pointer. Since, you have the
 return value there is no need to modify the pointer, allowing the caller
 to use it further if there was no match.
 >
 > Instead of using raw (unsafe) strings and memcmp you could use
 std::string objects and use equality operator and .compare function like
 this:
 > {{{
 >         // If the lengths are identical and the names match we're done.
 >         if (req_len == dom_len) {
 >             if (req_name == domain_name) {
 >                 // exact match, done
 >                 domain = map_pair.second;
 >                 return (true);
 >             }
 >         } else {
 >             // The fqdn is longer than the domain name.  Adjust the
 start
 >             // point of comparison by the excess in length.  Only do the
 >             // comparison if the adjustment lands on a boundary. This
 >             // prevents "onetwo.net" from matching "two.net".
 >             size_t offset = req_len - dom_len;
 >             if ((req_name[offset - 1] == '.')  &&
 >                 (fqdn.compare(offset, std::string::npos , domain_name)
 == 0)) {
 >                 // Fqdn contains domain name, keep it if its better than
 >                 // any we have matched so far.
 >                 if (dom_len > match_len) {
 >                     match_len = dom_len;
 >                     domain = map_pair.second;
 >                 }
 >             }
 >         }
 >
 > }}}
 >
 >

 Done.

 >
 > '''src/bin/d2/d2_messages.mes'''
 > The trash in the first line of this file:
 > {{{
 > /Users/tmark/ddns/build/trac3059/bind10
 > }}}
 >

 Oops. That's odd.  Got it.

 > General comment to all new messages... IMHO, it is unnecessary to add a
 double colon before the value included in the message. For example:
 > {{{
 > % DHCP_DDNS_AT_MAX_TRANSACTIONS application has: %1 queued requests but
 has reached maximum number of: %2 concurrent transactions
 > }}}
 > There is no need to double colon before %1 and %2.
 >

 Cleaned up the "single" colons ;)


 > '''src/bin/d2/d2_update_mgr.h'''
 > Typo in: !''... encounters an general error". It should be "...
 encounters a general error".
 >

 Got it.


 > NameChangeTransaction: deserves more comments. Especially, constructor's
 parameters should be described.
 >

 I did not spend much time commenting this class as it is a stub definition
 that will be replaced with the real thing shortly.  Unless you feel
 strongly, I'd rather wait.


 > Please remove
 > {{{
 > #if 0
 > ...
 > }}}

 I removed the conditional compile, but left the stats structure defined
 but unused.

 >
 > Also, should UpdateMgr be copyable?

 Done.

 >
 > Are there any functions in UpdateMgr that should be private? For
 example, I can see that the checkFinishedTransactions is called internally
 by sweep(). There is a question if it will be also called externally? Same
 for pickNextJob().
 >
 > hasTransaction has undocumented parameter !''key!''.

 Oops. Fixed.
 >
 > findTransaction, getQueueCount and getTransactionCount: can they be
 declared const?

 getQueueCount and getTransactionCount, yes.  findTransaction, which is
 used within removeTransaction, cannot be.  It would have to return a
 const_iterator, which then could not be used within removeTransaction.
 >
 > '''src/bin/d2/d2_update_mgr.cc'''
 > pickNextJob: Neat pick: it is better to use !''++index!'' rather than
 !''index++!''.


 >
 > '''src/bin/d2/tests/d2_cfg_mgr_unittests.cc'''
 > checkServer: Why not declare checkServer as void and perform
 !''expects!'' there like this:
 >
 > {{{
 >     EXPECT_TRUE(server);
 >
 >     // Check hostname.
 >     EXPECT_EQ(hostname, server->getHostname());
 > ...
 > }}}
 > etc. ?

 Because the  test log output is less precise that way.  If I do as you
 suggest
 the log loosk about like this:

 {{{
 [ RUN      ] DdnsDomainTest.DdnsDomainListParsing
 d2_cfg_mgr_unittests.cc:100: Failure
 Value of: server->getPort()
   Actual: 500
 Expected: port
 Which is: 700
 [  FAILED  ] DdnsDomainTest.DdnsDomainListParsing (0 ms)
 }}}

 The way it is written now, the log looks like this on a failure:

 {{{
 d2_cfg_mgr_unittests.cc:100: Failure
 Value of: server->getPort()
   Actual: 500
 Expected: port
 Which is: 700
 d2_cfg_mgr_unittests.cc:827: Failure
 Value of: checkServer(server, "", "127.0.0.5", 700)
   Actual: false
 Expected: true
 }}}

 Notice that you can easily see where it failed.  This is helpful when
 there are many
 calls to the same method in many tests.  I actually could make it return
 an Assertion result
 which is a technique I only recently learned.  However, at this point in
 the schedule there are more pressing things to do and these tests all
 function correctly.

 >
 > The !''Test!'' postfix in the name of each test case is redundant and
 could be omitted.
 >

 Changed.


 > '''src/lib/dhcp_ddns/ncr_msg.h'''
 > I don't understand the purpose of the < operator for DHCID. Can you
 explain how it will be used?
 >

 !TransactionList is currently implemented as a std::map, keyed by D2dhcid.
 Defining the "<" operator allows D2dhcid to be used as a key to std::map
 without having to define/supply a compare class.

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