BIND 10 #3156: Create separate base-classes for the Finite State Machine logic in NameChangeTransaction class

[BIND 10 Development]

#3156: Create separate base-classes for the Finite State Machine logic in
NameChangeTransaction class
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  low           |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           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 tomek):

 * owner:  tomek => tmark


Comment:

 General comment: state_model is generic and can be used by other
 components, not only d2. It should be moved somewhere to src/lib.
 src/lib/util would be my initial pick, but it should probably be
 consulted with folks on bind10 chatroom.

 This should be reflected by the ChangeLog entry.

 Please make sure you keep the diagrams, so they can be used for a nice
 doxygen description. I think there's a ticket for that already, isn't it?



 '''state_model.h'''

 File comment mentions nc_trans.h.

 "No work can done" => "No work can be done"

 argument which is "posts" => argument which it "posts"

 "runModel repeats this process until it either NOP_EVT or END_EVT events
 are posted."
 What about FAIL_EVT?

 "must wait or ends ." => Extra space before dot.

 "To progress from from one state" => Extra "from"

 "a state and an a event" => Remove extra article.

 "postEvent method is may be" => Either is or may, not both.

 StateModel::SM_STATE_MAX - shouldn't it be SM_STATE_MIN as this is the
 minimal state that derived classes should define?

 runModel(): The fact that state handlers may run multiple interations of a
 state
 change is a bit worrisome, because it is now impossible to detect
 potential infinite loops in FSM logic. This brings in a halting
 problem to the scene. Halting problem is NP-hard, unfortunately. Since
 I don't have a better solution (if I had, I would likely win Turing
 Award in the process), let the code stay as it is now.

 endModel(): "It is should be called"
 Either "is called" or "should be called".

 verifyStateHandlerMap():
 "a implementation" => "an implementation"

 onModelFailure(): I think you should provide some generic error handler.
 It would make
 derived implementations easier.

 getStateLabel(), getEventLabel():
 It would be better to implement a constructor that takes 2 map<int,
 string> objects. The methods could be defined in base class and
 wounldn't have to be defined in each derived class. If there are
 problems with passing map, then perhaps a static array of
 {{{
 struct {
     int;
     const char*;
     StateHandler;
 }
 }}}
 could be passed. That would eliminate the need to implement
 getStateLabel(), getEventLabel() and probably other as well.

 addToMap: This name is confusing as it is not clear what the map is,
 without looking into the doc. addToStateMap or simply addState() would
 be more appropriate.

 '''state_model_unittests.cc'''
 Do you think it would be useful to check if runModel() throws if there
 was no startModel() called?

 Also, I think the model should throw if you call addState() after
 startModel(). Otherwise we could allow a FSM that modifies itself
 during run. That would encourage shady tricks.


 '''nc_trans.h'''
 Why do you need NameChangeTransactionError anymore? Wouldn't
 StateModelError suffice?

 verifyStateHandlerMap(): This comment mostly repeats what is said for
 StateModel::verifyStateHandlerMap(). Perhaps you could trim down the
 comments in NameChangeTransaction.

 If you agree to my proposal for passing map of states as an array or
 map, then several methods wouldn't be necessary in
 NameChangeTransaction.

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