BIND 10 #3088: Develop NameRemoveTransaction class for b10-dhcp-ddns

BIND 10 Development do-not-reply at isc.org
Mon Dec 16 14:39:47 UTC 2013


#3088: Develop NameRemoveTransaction class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:  DHCP-
            Keywords:  Task# 4.3     |  Kea1.0-alpha
           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:

 Reviewed commit 70941f1375063c52a43a10831dc9707c6303f55a

 In general a very good piece of code. I didn't find anything flagrant.

 '''!ChangeLog'''
 When referring to !''DHCP_DDNS design!'', it may be appropriate to give a
 link where this design can be found. I am not sure what the practice was
 so far - and what happens if the link has to change for any reason. On the
 other hand, referring to the design without saying were it can be found
 may not make sense either.

 Also, please include a ticket and commit number for the changelog entry.

 '''src/bin/d2/d2_messages.mes'''
 DHCP_DDNS_FORWARD_REMOVE_RRS_BUILD_FAILURE: Suggest that you put comma
 between %1 and reason.

 DHCP_DDNS_REMOVE_FAILED: Typo !''There!''. Should be !''The!''.

 '''src/bin/d2/nc_add.cc'''

 verifyEvents, verifyStates: it is good that you expanded NCT abbrevation
 to the long form. But I think it was also good to have the extended
 description what the verification is about, i.e. that you check that
 states and events are defined.

 processAddFailedHandler: When the NO_MORE_SERVERS_EVT occurs you log the
 same error as in the case of UPDATE_FAILED_EVT. Question: is it always the
 case that the occurence of NO_MORE_SERVERS_EVT is an error? For example:
 if I don't have any servers configured yet. Is it an error? I am wondering
 if the log message should be differentiated for the actual update falure
 and for the case when there are no servers to use. What do you think?

 BTW, what is the code's behavior if there is no server configured? Is
 there a warning message issued somewhere that no servers are configured?
 If not, simply priting that update has failed without making a point that
 that was because no servers are yet configured may be misleading - or at
 least doesn't give a lot of information. :)

 '''src/bin/d2/nc_remove.cc'''

 Constructor: The io_service could be sanity checked for being a NULL
 pointer. Same for forward_domain and reverse_domain.

 verifyStates: it would be useful to extend the commentary inside this
 function to say which states are verified in the parent class. Apparently,
 the common states such as SELECTING_FWD_SERVER_ST are verified in the
 parent class, but I wasn't certain until I actually went to the parent
 class'es code.

 '''src/bin/d2/nc_remove.h'''

 There is a couple of typos being a result of copy-paste from the
 !NameAddTransaction. For example, for the defineEvents function: !''
 Removes events defined by !NameRemoveTransaction to the event set!''.  It
 should be !''... from the event set!''. Please check the whole header for
 such errors.

 readyHandler: !''... state model execution.h!'' - spurious !''h!''?

 !NameRemoveTransactionPtr: the commentary should say: !''Defines a pointer
 to a !NameRemoveTransaction!''.

 '''src/bin/d2/nc_trans.cc'''

 addLeaseAddressRdata: The following line:
 {{{
 rrset->addRdata(rdata);
 }}}
 could be common for both V4 and V6 case. So, the code could look like
 this:
 {{{
         dns::rdata::ConstRdataPtr rdata;
         if (ncr_->isV4()) {
             rdata.reset(new dns::rdata::in::A(ncr_->getIpAddress()));
         } else {
             rdata.reset(new dns::rdata::in::AAAA(ncr_->getIpAddress()));
         }
         rrset->addRdata(rdata);
 }}}

 addPtrRdata: Why don't you just use the smart pointer constructor to
 initialize the pointer in the point of declaration:
 {{{
 dns::rdata::ConstRdataPtr rdata(new
 dns::rdata::generic::PTR(getNcr()->getFqdn()));
 }}}
 Otherwise, you call default constructor and then reset the pointer which
 has some performance implications. Similar comment applies to
 addDhcidRdata.

 '''src/bin/d2/nc_trans.h'''

 retryTransition: the name of the parameter has been changed. Please change
 the name in the doxygen documentation too.

 '''src/bin/d2/tests/nc_remove_unittests.cc'''

 I compared the !NameAddStub and !NameRemoveStub and it occurs to me that
 they are very similar. They differ by the object type being created:
 !NameAddTransaction vs !NameRemoveTransaction, but even there, they have
 the same constructors. The methods like fakeReponse are identical. I
 wonder if it makes sense to unify the test classes.

 One of the differences found is the word !''exercise!'' instead of
 !''exercised!'' in the !NameRemoveStub. :) You may want to correct this
 typo.

 Also, in the !NameRemoveTransactionTest you decided to use the constructor
 to create a transaction object (see makeTransaction4), instead of JSON (as
 in case of !NameAddStub). Are you planning to change this for the
 !NameAddTransactionTest?

 construction test: you may want to implement tests for NULL pointers being
 passed to the constructor.

 construction test: The alignment of the parameters passed to contructor is
 odd.

 buildRemoveFwdAddressRequest: In this part:
 {{{
     ASSERT_NO_THROW(name_remove = makeTransaction4(FORWARD_CHG));
     (name_remove->buildRemoveFwdAddressRequest());
     ASSERT_NO_THROW(name_remove->buildRemoveFwdAddressRequest());

 }}}
 the second line is supposed to be removed?

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


More information about the bind10-tickets mailing list