BIND 10 #3059: Create initial UpdateMgr class

[BIND 10 Development]

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

 * owner:  marcin => tmark


Comment:

 '''src/bin/d2/d2_cfg_mgr.h'''
 Technically, functions: reverseIpAddress, reverseV4Address and
 reverseV6Address could be static.

 '''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.

 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.

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

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

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

 '''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;
                 }
             }
         }

 }}}



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

 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.

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

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

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

 Also, should UpdateMgr be copyable?

 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!''.

 findTransaction, getQueueCount and getTransactionCount: can they be
 declared const?

 '''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. ?

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

 '''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?

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