[3156] Review comments addressed: part II - d2::StateMode revisions.

This commit replaces the state specification mechanism in d2::StateModel
with one derived from LabeledValue and LabeledValueSet.

src/bin/d2/d2_messages.mes

@@ -253,7 +253,7 @@ in event loop.
 This is informational message issued when the application has been instructed
 to shut down by the controller.
 
-% DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR application encountered an unexpected error while carrying out a NameChangeRequest: %1 , %2
+% DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR application encountered an unexpected error while carrying out a NameChangeRequest: %1
 This is error message issued when the application fails to process a
 NameChangeRequest correctly. Some or all of the DNS updates requested as part
 of this update did not succeed. This is a programmatic error and should be

src/bin/d2/nc_trans.cc

@@ -83,7 +83,10 @@ NameChangeTransaction::operator()(DNSClient::Status status) {
 
 void
 NameChangeTransaction::defineEvents() {
+    // Call superclass impl first.
     StateModel::defineEvents();
+
+    // Define NCT events.
     defineEvent(SELECT_SERVER_EVT, "SELECT_SERVER_EVT");
     defineEvent(SERVER_SELECTED_EVT, "SERVER_SELECTED_EVT");
     defineEvent(SERVER_IO_ERROR_EVT, "SERVER_IO_ERROR_EVT");
@@ -95,7 +98,10 @@ NameChangeTransaction::defineEvents() {
 
 void
 NameChangeTransaction::verifyEvents() {
+    // Call superclass impl first.
     StateModel::verifyEvents();
+
+    // Verify NCT events.
     getEvent(SELECT_SERVER_EVT);
     getEvent(SERVER_SELECTED_EVT);
     getEvent(SERVER_IO_ERROR_EVT);
@@ -106,17 +112,31 @@ NameChangeTransaction::verifyEvents() {
 }
 
 void
-NameChangeTransaction::verifyStateHandlerMap() {
-    getStateHandler(READY_ST);
-    getStateHandler(SELECTING_FWD_SERVER_ST);
-    getStateHandler(SELECTING_REV_SERVER_ST);
-    getStateHandler(PROCESS_TRANS_OK_ST);
-    getStateHandler(PROCESS_TRANS_FAILED_ST);
+NameChangeTransaction::defineStates() {
+    // Call superclass impl first.
+    StateModel::defineStates();
+    // This class is "abstract" in that it does not supply handlers for its
+    // states, derivations must do that therefore they must define them.
+}
+
+void
+NameChangeTransaction::verifyStates() {
+    // Call superclass impl first.
+    StateModel::verifyStates();
+
+    // Verify NCT states. This ensures that derivations provide the handlers.
+    getState(READY_ST);
+    getState(SELECTING_FWD_SERVER_ST);
+    getState(SELECTING_REV_SERVER_ST);
+    getState(PROCESS_TRANS_OK_ST);
+    getState(PROCESS_TRANS_FAILED_ST);
 }
 
 void
-NameChangeTransaction::onModelFailure() {
+NameChangeTransaction::onModelFailure(const std::string& explanation) {
     setNcrStatus(dhcp_ddns::ST_FAILED);
+    LOG_ERROR(dctl_logger, DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR)
+                  .arg(explanation);
 }
 
 void
@@ -226,32 +246,5 @@ NameChangeTransaction::getReverseChangeCompleted() const {
     return (reverse_change_completed_);
 }
 
-const char*
-NameChangeTransaction::getStateLabel(const int state) const {
-    const char* str = "Unknown";
-    switch(state) {
-    case READY_ST:
-        str = "NameChangeTransaction::READY_ST";
-        break;
-    case SELECTING_FWD_SERVER_ST:
-        str = "NameChangeTransaction::SELECTING_FWD_SERVER_ST";
-        break;
-    case SELECTING_REV_SERVER_ST:
-        str = "NameChangeTransaction::SELECTING_REV_SERVER_ST";
-        break;
-    case PROCESS_TRANS_OK_ST:
-        str = "NameChangeTransaction::PROCESS_TRANS_OK_ST";
-        break;
-    case PROCESS_TRANS_FAILED_ST:
-        str = "NameChangeTransaction::PROCESS_TRANS_FAILED_ST";
-        break;
-    default:
-        str = StateModel::getStateLabel(state);
-        break;
-    }
-
-    return (str);
-}
-
 } // namespace isc::d2
 } // namespace isc

src/bin/d2/nc_trans.h

@@ -182,12 +182,12 @@ public:
     virtual void operator()(DNSClient::Status status);
 
 protected:
-    /// @brief Adds events defined by NameChangeTransaction to the event set. 
+    /// @brief Adds events defined by NameChangeTransaction to the event set.
     ///
     /// This method adds the events common to NCR transaction processing to
     /// the set of define events.  It invokes the superclass's implementation
     /// first to maitain the hierarchical chain of event defintion.
-    /// Derivations of NameChangeTransaction must invoke its implementation 
+    /// Derivations of NameChangeTransaction must invoke its implementation
     /// in like fashion.
     ///
     /// @throw StateModelError if an event definition is invalid or a duplicate.
@@ -198,58 +198,33 @@ protected:
     /// This method verifies that the events defined by both the superclass and
     /// this class are defined.  As with defineEvents, this method calls the
     /// superclass's implementation first, to verify events defined by it and
-    /// then this implementation to verify events defined by 
+    /// then this implementation to verify events defined by
     /// NameChangeTransaction.
     ///
-    /// @throw StateModelError if an event value is undefined. 
+    /// @throw StateModelError if an event value is undefined.
     virtual void verifyEvents();
 
-    /// @brief Populates the map of state handlers.
+    /// @brief Adds states defined by NameChangeTransaction to the state set.
     ///
-    /// This method is used by derivations to construct a map of states to
-    /// their appropriate state handlers (bound method pointers).  It is
-    /// invoked at the beginning of startTransaction().
+    /// This method adds the states common to NCR transaction processing to
+    /// the dictionary of states.  It invokes the superclass's implementation
+    /// first to maitain the hierarchical chain of state defintion.
+    /// Derivations of NameChangeTransaction must invoke its implementation
+    /// in like fashion.
     ///
-    /// Implementations should use the addToMap() method add entries to
-    /// the map.
-    /// @todo This method should be pure virtual but until there are
-    /// derivations for the update manager to use, we will provide a
-    /// temporary empty, implementation.  If we make it pure virtual now
-    /// D2UpdateManager will not compile.
-    virtual void initStateHandlerMap() {};
+    /// @throw StateModelError if an state definition is invalid or a duplicate.
+    virtual void defineStates();
 
-    /// @brief Validates the contents of the state handler map.
-    ///
-    /// This method is invoked immediately after initStateHandlerMap and
-    /// verifies that the state map includes handlers for all of the states
-    /// defined by NameChangeTransaction.  If the map is determined to be
-    /// invalid this method will throw a NameChangeTransactionError.
-    ///
-    /// Derivations should ALSO provide an implementation of this method. That
-    /// implementation should invoke this method, as well as verifying that all
-    /// of the derivation's states have handlers.
-    ///
-    /// A derivation's implementation of this function might look as follows:
-    ///
-    /// @code
+    /// @brief Validates the contents of the set of states.
     ///
-    ///     class DerivedTrans : public NameChangeTransaction {
-    ///         :
-    ///         void verifyStateHandlerMap() {
-    ///             // Verify derivations' states:
-    ///             getStateHandler(SOME_DERIVED_STATE_1);
-    ///             getStateHandler(SOME_DERIVED_STATE_2);
-    ///             :
-    ///             getStateHandler(SOME_DERIVED_STATE_N);
-    ///
-    ///             // Verify handlers for NameChangeTransaction states:
-    ///             NameChangeTransaction::verifyStateHandlerMap();
-    ///         }
-    ///
-    /// @endcode
+    /// This method verifies that the states defined by both the superclass and
+    /// this class are defined.  As with defineStates, this method calls the
+    /// superclass's implementation first, to verify states defined by it and
+    /// then this implementation to verify states defined by
+    /// NameChangeTransaction.
     ///
-    /// @throw NameChangeTransactionError if the map is invalid.
-    virtual void verifyStateHandlerMap();
+    /// @throw StateModelError if an event value is undefined.
+    virtual void verifyStates();
 
     /// @brief Handler for fatal model execution errors.
     ///
@@ -260,7 +235,9 @@ protected:
     /// error occurs the transaction is deemed inoperable, and futher model
     /// execution cannot be performed.  It marks the transaction as failed by
     /// setting the NCR status to dhcp_ddns::ST_FAILED
-    virtual void onModelFailure();
+    ///
+    /// @param explanation is text detailing the error
+    virtual void onModelFailure(const std::string& explanation);
 
     /// @brief Sets the update status to the given status value.
     ///
@@ -388,44 +365,6 @@ public:
     /// @return True if the reverse change has been completed, false otherwise.
     bool getReverseChangeCompleted() const;
 
-    /// @brief Converts a state value into a text label.
-    ///
-    /// This method supplies labels for NameChangeTransaction's predefined
-    /// states. It is declared virtual to allow derivations to embed a call to
-    /// this method within their own implementation which would define labels
-    /// for its states.  An example implementation might look like the
-    /// following:
-    /// @code
-    ///
-    /// class DerivedTrans : public NameChangeTransaction {
-    ///     :
-    ///     static const int EXAMPLE_1_ST = NCT_STATE_MAX + 1;
-    ///     :
-    ///     const char* getStateLabel(const int state) const {
-    ///         const char* str = "Unknown";
-    ///         switch(state) {
-    ///         case EXAMPLE_1_ST:
-    ///             str = "DerivedTrans::EXAMPLE_1_ST";
-    ///             break;
-    ///         :
-    ///         default:
-    ///             // Not a derived state, pass it to NameChangeTransaction's
-    ///             // method.
-    ///             str = NameChangeTransaction::getStateLabel(state);
-    ///             break;
-    ///         }
-    ///
-    ///         return (str);
-    ///      }
-    ///
-    /// @endcode
-    ///
-    /// @param state is the state for which a label is desired.
-    ///
-    /// @return Returns a const char* containing the state label or
-    /// "Unknown" if the value cannot be mapped.
-    virtual const char* getStateLabel(const int state) const;
-
 private:
     /// @brief The IOService which should be used to for IO processing.
     isc::asiolink::IOService& io_service_;

src/bin/d2/state_model.cc

@@ -20,6 +20,53 @@
 namespace isc {
 namespace d2 {
 
+/********************************** State *******************************/
+
+State::State(const int value, const char* label, StateHandler handler)
+        : LabeledValue(value, label), handler_(handler) {
+}
+
+State::~State() {
+}
+
+void
+State::run() {
+        (handler_)();
+}
+
+/********************************** StateSet *******************************/
+
+StateSet::StateSet() {
+}
+
+StateSet::~StateSet() {
+}
+
+void
+StateSet::add(const int value, const char* label, StateHandler handler) {
+    try {
+        LabeledValueSet::add(LabeledValuePtr(new State(value, label, handler)));
+    } catch (const std::exception& ex) {
+        isc_throw(StateModelError, "StateSet: cannot add state :" << ex.what());
+    }
+
+}
+
+const StatePtr
+StateSet::getState(int value) {
+    if (!isDefined(value)) {
+        isc_throw(StateModelError," StateSet: state is undefined");
+    }
+
+    // Since we have to use dynamic casting, to get a state pointer
+    // we can't return a reference.
+    StatePtr state = boost::dynamic_pointer_cast<State>(get(value));
+    return (state);
+}
+
+/********************************** StateModel *******************************/
+
+
 // Common state model states
 const int StateModel::NEW_ST;
 const int StateModel::END_ST;
@@ -34,10 +81,10 @@ const int StateModel::FAIL_EVT;
 
 const int StateModel::SM_EVENT_MAX;
 
-StateModel::StateModel() : state_handlers_(), state_(NEW_ST),
-                          prev_state_(NEW_ST), last_event_(NOP_EVT),
-                          next_event_(NOP_EVT), on_entry_flag_(false),
-                          on_exit_flag_(false) {
+StateModel::StateModel() : events_(), states_(), dictionaries_initted_(false),
+                          curr_state_(NEW_ST), prev_state_(NEW_ST),
+                          last_event_(NOP_EVT), next_event_(NOP_EVT),
+                          on_entry_flag_(false), on_exit_flag_(false) {
 }
 
 StateModel::~StateModel(){
@@ -45,6 +92,7 @@ StateModel::~StateModel(){
 
 void
 StateModel::startModel(const int start_state) {
+    // First let's build and verify the dictionary of events.
     try {
         defineEvents();
         verifyEvents();
@@ -52,20 +100,29 @@ StateModel::startModel(const int start_state) {
         isc_throw(StateModelError, "Event set is invalid: " << ex.what());
     }
 
-    // Initialize the state handler map first.
-    initStateHandlerMap();
+    // Next let's build and verify the dictionary of states.
+    try {
+        defineStates();
+        verifyStates();
+    } catch (const std::exception& ex) {
+        isc_throw(StateModelError, "State set is invalid: " << ex.what());
+    }
 
-    // Test validity of the handler map. This provides an opportunity to
-    // sanity check the map prior to attempting to execute the model.
-    verifyStateHandlerMap();
+    // Record that we are good to go.
+    dictionaries_initted_ = true;
 
-    // Set the current state to startng state and enter the run loop.
+    // Set the current state to starting state and enter the run loop.
     setState(start_state);
     runModel(START_EVT);
 }
 
 void
 StateModel::runModel(unsigned int run_event) {
+    /// If the dictionaries aren't built bail out.
+    if (!dictionaries_initted_) {
+       abortModel("runModel invoked before model has been initialized");
+    }
+
     try {
         // Seed the loop with the given event as the next to process.
         postNextEvent(run_event);
@@ -73,32 +130,35 @@ StateModel::runModel(unsigned int run_event) {
             // Invoke the current state's handler.  It should consume the
             // next event, then determine what happens next by setting
             // current state and/or the next event.
-            (getStateHandler(state_))();
+            getState(curr_state_)->run();
 
             // Keep going until a handler sets next event to a NOP_EVT.
         } while (!isModelDone() && getNextEvent() != NOP_EVT);
     }
     catch (const std::exception& ex) {
-        // The model has suffered an unexpected exception. This constitutes a
+        // The model has suffered an unexpected exception. This constitutes
         // a model violation and indicates a programmatic shortcoming.
         // In theory, the model should account for all error scenarios and
-        // deal with them accordingly.  Log it and transition to END_ST with
-        // FAILED_EVT via abortModel.
-        LOG_ERROR(dctl_logger, DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR)
-                  .arg(ex.what()).arg(getContextStr());
-        abortModel();
+        // deal with them accordingly.  Transition to END_ST with FAILED_EVT
+        // via abortModel.
+        abortModel(ex.what());
     }
 }
 
+void
+StateModel::nopStateHandler() {
+}
+
+
 void
 StateModel::defineEvent(unsigned int event_value, const char* label) {
     if (!isModelNew()) {
-        // Don't allow for self-modifying maps.
+        // Don't allow for self-modifying models.
         isc_throw(StateModelError, "Events may only be added to a new model."
                    << event_value << " - " << label);
     }
 
-    // add method may throw on duplicate or empty label.  
+    // Attempt to add the event to the set.
     try {
         events_.add(event_value, label);
     } catch (const std::exception& ex) {
@@ -109,52 +169,41 @@ StateModel::defineEvent(unsigned int event_value, const char* label) {
 const EventPtr&
 StateModel::getEvent(unsigned int event_value) {
     if (!events_.isDefined(event_value)) {
-        isc_throw(StateModelError, 
+        isc_throw(StateModelError,
                   "Event value is not defined:" << event_value);
     }
 
     return (events_.get(event_value));
 }
 
-StateHandler
-StateModel::getStateHandler(unsigned int state) {
-    StateHandlerMap::iterator it = state_handlers_.find(state);
-    // It is wrong to try to find a state that isn't mapped.
-    if (it == state_handlers_.end()) {
-        isc_throw(StateModelError, "No handler for state: "
-                  << state << " - " << getStateLabel(state));
-    }
-
-    return ((*it).second);
-}
-
 void
-StateModel::addToStateHandlerMap(unsigned int state, StateHandler handler) {
+StateModel::defineState(unsigned int state_value, const char* label,
+    StateHandler handler) {
     if (!isModelNew()) {
         // Don't allow for self-modifying maps.
-        isc_throw(StateModelError,
-                  "State handler mappings can only be added to a new model."
-                   << state << " - " << getStateLabel(state));
+        isc_throw(StateModelError, "States may only be added to a new model."
+                   << state_value << " - " << label);
     }
 
-    StateHandlerMap::iterator it = state_handlers_.find(state);
-    // Check for a duplicate attempt.  State's can't have more than one.
-    if (it != state_handlers_.end()) {
-        isc_throw(StateModelError,
-                  "Attempted duplicate entry in state handler map, state: "
-                   << state << " - " << getStateLabel(state));
+    // Attempt to add the state to the set.
+    try {
+        states_.add(state_value, label, handler);
+    } catch (const std::exception& ex) {
+        isc_throw(StateModelError, "Error adding state: " << ex.what());
     }
+}
 
-    // Do not allow handlers for special states fo NEW_ST and END_ST.
-    if (state == NEW_ST || state == END_ST) {
-        isc_throw(StateModelError, "A handler is not supported for state: "
-                  << state << " - " << getStateLabel(state));
+const StatePtr
+StateModel::getState(unsigned int state_value) {
+    if (!states_.isDefined(state_value)) {
+        isc_throw(StateModelError,
+                  "State value is not defined:" << state_value);
     }
 
-    state_handlers_[state] = handler;
+    return (states_.getState(state_value));
 }
 
-void 
+void
 StateModel::defineEvents() {
     defineEvent(NOP_EVT, "NOP_EVT");
     defineEvent(START_EVT, "START_EVT");
@@ -170,6 +219,19 @@ StateModel::verifyEvents() {
     getEvent(FAIL_EVT);
 }
 
+void
+StateModel::defineStates() {
+    defineState(NEW_ST, "NEW_ST",
+                boost::bind(&StateModel::nopStateHandler, this));
+    defineState(END_ST, "END_ST",
+                boost::bind(&StateModel::nopStateHandler, this));
+}
+
+void
+StateModel::verifyStates() {
+    getState(NEW_ST);
+    getState(END_ST);
+}
 
 void
 StateModel::transition(unsigned int state, unsigned int event) {
@@ -183,33 +245,37 @@ StateModel::endModel() {
 }
 
 void
-StateModel::abortModel() {
+StateModel::abortModel(const std::string& explanation) {
     transition(END_ST, FAIL_EVT);
-    onModelFailure();
+
+    std::ostringstream stream ;
+    stream << explanation << " : " << getContextStr();
+    onModelFailure(stream.str());
 }
 
 void
 StateModel::setState(unsigned int state) {
-    // If the new state isn't NEW_ST or END_ST, make sure it has a handler.
-    if ((state && state != NEW_ST && state != END_ST)
-        && (state_handlers_.end() == state_handlers_.find(state))) {
-        isc_throw(StateModelError, "Attempt to set state to invalid stat :"
-                  << state << "=" << getStateLabel(state));
+    if (state != END_ST && !states_.isDefined(state)) {
+        isc_throw(StateModelError,
+                  "Attempt to set state to an undefined value: " << state );
     }
 
-    prev_state_ = state_;
-    state_ = state;
+    prev_state_ = curr_state_;
+    curr_state_ = state;
 
     // Set the "do" flags if we are transitioning.
-    on_entry_flag_ = ((state != END_ST) && (prev_state_ != state_));
+    on_entry_flag_ = ((state != END_ST) && (prev_state_ != curr_state_));
+
     // At this time they are calculated the same way.
     on_exit_flag_ = on_entry_flag_;
 }
 
 void
 StateModel::postNextEvent(unsigned int event_value) {
+    // Check for FAIL_EVT as special case of model error before events are
+    // defined.
     if (event_value != FAIL_EVT && !events_.isDefined(event_value)) {
-        isc_throw(StateModelError, 
+        isc_throw(StateModelError,
                   "Attempt to post an undefined event, value: " << event_value);
     }
 
@@ -232,8 +298,8 @@ StateModel::doOnExit() {
 }
 
 unsigned int
-StateModel::getState() const {
-    return (state_);
+StateModel::getCurrState() const {
+    return (curr_state_);
 }
 
 unsigned int
@@ -252,12 +318,12 @@ StateModel::getNextEvent() const {
 }
 bool
 StateModel::isModelNew() const {
-    return (state_ == NEW_ST);
+    return (curr_state_ == NEW_ST);
 }
 
 bool
 StateModel::isModelRunning() const {
-    return ((state_ != NEW_ST) && (state_ != END_ST));
+    return ((curr_state_ != NEW_ST) && (curr_state_ != END_ST));
 }
 
 bool
@@ -267,7 +333,7 @@ StateModel::isModelWaiting() const {
 
 bool
 StateModel::isModelDone() const {
-    return (state_ == END_ST);
+    return (curr_state_ == END_ST);
 }
 
 bool
@@ -277,26 +343,19 @@ StateModel::didModelFail() const {
 
 const char*
 StateModel::getStateLabel(const int state) const {
-    const char* str = "Unknown";
-    switch(state) {
-    case NEW_ST:
-        str = "StateModel::NEW_ST";
-        break;
-    case END_ST:
-        str = "StateModel::END_ST";
-        break;
-    default:
-        break;
-    }
+    return (states_.getLabel(state));
+}
 
-    return (str);
+const char*
+StateModel::getEventLabel(const int event) const {
+    return (events_.getLabel(event));
 }
 
 std::string
 StateModel::getContextStr() const {
     std::ostringstream stream;
     stream << "current state: [ "
-            << state_ << " " << getStateLabel(state_)
+            << curr_state_ << " " << getStateLabel(curr_state_)
             << " ] next event: [ "
             << next_event_ << " " << getEventLabel(next_event_) << " ]";
     return(stream.str());
@@ -312,10 +371,5 @@ StateModel::getPrevContextStr() const {
     return(stream.str());
 }
 
-const char*
-StateModel::getEventLabel(const int event) const {
-    return (events_.getLabel(event));
-}
-
 } // namespace isc::d2
 } // namespace isc

src/bin/d2/tests/nc_trans_unittests.cc

@@ -131,65 +131,63 @@ public:
         }
     }
 
+    /// @brief Construct the event dictionary.
     virtual void defineEvents() {
+        // Invoke the base call implementation first.
         NameChangeTransaction::defineEvents();
+
+        // Define our events.