diff --git a/src/bin/d2/nc_add.cc b/src/bin/d2/nc_add.cc index 3114470f81..f9ab340a24 100644 --- a/src/bin/d2/nc_add.cc +++ b/src/bin/d2/nc_add.cc @@ -116,9 +116,9 @@ NameAddTransaction::verifyStates() { NameChangeTransaction::verifyStates(); // Verify NameAddTransaction states by attempting to fetch them. - getState(ADDING_FWD_ADDRS_ST); - getState(REPLACING_FWD_ADDRS_ST); - getState(REPLACING_REV_PTRS_ST); + getStateInternal(ADDING_FWD_ADDRS_ST); + getStateInternal(REPLACING_FWD_ADDRS_ST); + getStateInternal(REPLACING_REV_PTRS_ST); } void diff --git a/src/bin/d2/nc_remove.cc b/src/bin/d2/nc_remove.cc index ec6fd5c2c3..f80d654709 100644 --- a/src/bin/d2/nc_remove.cc +++ b/src/bin/d2/nc_remove.cc @@ -120,9 +120,9 @@ NameRemoveTransaction::verifyStates() { NameChangeTransaction::verifyStates(); // Verify NameRemoveTransaction states by attempting to fetch them. - getState(REMOVING_FWD_ADDRS_ST); - getState(REMOVING_FWD_RRS_ST); - getState(REMOVING_REV_PTRS_ST); + getStateInternal(REMOVING_FWD_ADDRS_ST); + getStateInternal(REMOVING_FWD_RRS_ST); + getStateInternal(REMOVING_REV_PTRS_ST); } void diff --git a/src/bin/d2/nc_trans.cc b/src/bin/d2/nc_trans.cc index 6fa8249b40..cfcb99f6c8 100644 --- a/src/bin/d2/nc_trans.cc +++ b/src/bin/d2/nc_trans.cc @@ -244,11 +244,11 @@ NameChangeTransaction::verifyStates() { 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); + getStateInternal(READY_ST); + getStateInternal(SELECTING_FWD_SERVER_ST); + getStateInternal(SELECTING_REV_SERVER_ST); + getStateInternal(PROCESS_TRANS_OK_ST); + getStateInternal(PROCESS_TRANS_FAILED_ST); } void diff --git a/src/bin/d2/tests/nc_trans_unittests.cc b/src/bin/d2/tests/nc_trans_unittests.cc index 33fc24be64..730a1f3812 100644 --- a/src/bin/d2/tests/nc_trans_unittests.cc +++ b/src/bin/d2/tests/nc_trans_unittests.cc @@ -221,7 +221,7 @@ public: NameChangeTransaction::verifyStates(); // Check our states. - getState(DOING_UPDATE_ST); + getStateInternal(DOING_UPDATE_ST); } // Expose the protected methods to be tested. diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index cb4d66db52..6a0950592f 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -337,7 +337,7 @@ HAService::partnerInMaintenanceStateHandler() { switch (communication_state_->getPartnerState()) { case HA_UNAVAILABLE_ST: verboseTransition(HA_PARTNER_DOWN_ST); - + /* Falls through. */ default: postNextEvent(NOP_EVT); } diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index e805bff432..58e2d57e15 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -731,7 +731,6 @@ IfaceMgr::startDHCPReceiver(const uint16_t family) { dhcp_receiver_.reset(new WatchedThread()); dhcp_receiver_->start(boost::bind(&IfaceMgr::receiveDHCP4Packets, this)); - break; case AF_INET6: // If the queue doesn't exist, packet queing has been configured @@ -1211,7 +1210,8 @@ Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* // Note the external socket to call its callback without // the lock taken so it can be deleted. ex_sock = s; - break; } + break; + } } } diff --git a/src/lib/util/labeled_value.cc b/src/lib/util/labeled_value.cc index cf0fc6d940..5d16b6f628 100644 --- a/src/lib/util/labeled_value.cc +++ b/src/lib/util/labeled_value.cc @@ -74,14 +74,14 @@ LabeledValueSet::add(LabeledValuePtr entry) { isc_throw(LabeledValueError, "value: " << value << " is already defined as: " << getLabel(value)); - } + } - map_[entry->getValue()]=entry; + map_[value] = entry; } void LabeledValueSet::add(const int value, const std::string& label) { - add (LabeledValuePtr(new LabeledValue(value,label))); + add(LabeledValuePtr(new LabeledValue(value,label))); } const LabeledValuePtr& diff --git a/src/lib/util/state_model.cc b/src/lib/util/state_model.cc index f4b511a653..a4fea31c34 100644 --- a/src/lib/util/state_model.cc +++ b/src/lib/util/state_model.cc @@ -113,7 +113,7 @@ 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"); + abortModel("runModel invoked before model has been initialized"); } try { @@ -127,8 +127,7 @@ StateModel::runModel(unsigned int run_event) { // Keep going until a handler sets next event to a NOP_EVT. } while (!isModelDone() && getNextEvent() != NOP_EVT); - } - catch (const std::exception& ex) { + } catch (const std::exception& ex) { // 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 @@ -144,6 +143,10 @@ StateModel::nopStateHandler() { void StateModel::initDictionaries() { + std::lock_guard lock(*mutex_); + if (dictionaries_initted_) { + isc_throw(StateModelError, "Dictionaries already initialized"); + } // First let's build and verify the dictionary of events. try { defineEvents(); @@ -166,7 +169,7 @@ StateModel::initDictionaries() { void StateModel::defineEvent(unsigned int event_value, const std::string& label) { - if (!isModelNew()) { + if (!isModelNewInternal()) { // Don't allow for self-modifying models. isc_throw(StateModelError, "Events may only be added to a new model." << event_value << " - " << label); @@ -193,7 +196,7 @@ StateModel::getEvent(unsigned int event_value) { void StateModel::defineState(unsigned int state_value, const std::string& label, StateHandler handler, const StatePausing& state_pausing) { - if (!isModelNew()) { + if (!isModelNewInternal()) { // Don't allow for self-modifying maps. isc_throw(StateModelError, "States may only be added to a new model." << state_value << " - " << label); @@ -209,6 +212,12 @@ StateModel::defineState(unsigned int state_value, const std::string& label, const StatePtr StateModel::getState(unsigned int state_value) { + std::lock_guard lock(*mutex_); + return getStateInternal(state_value); +} + +const StatePtr +StateModel::getStateInternal(unsigned int state_value) { if (!states_.isDefined(state_value)) { isc_throw(StateModelError, "State value is not defined:" << state_value); @@ -243,8 +252,8 @@ StateModel::defineStates() { void StateModel::verifyStates() { - getState(NEW_ST); - getState(END_ST); + getStateInternal(NEW_ST); + getStateInternal(END_ST); } void @@ -254,8 +263,9 @@ StateModel::onModelFailure(const std::string&) { void StateModel::transition(unsigned int state, unsigned int event) { - setState(state); - postNextEvent(event); + std::lock_guard lock(*mutex_); + setStateInternal(state); + postNextEventInternal(event); } void @@ -265,6 +275,7 @@ StateModel::endModel() { void StateModel::unpauseModel() { + std::lock_guard lock(*mutex_); paused_ = false; } @@ -280,6 +291,11 @@ StateModel::abortModel(const std::string& explanation) { void StateModel::setState(unsigned int state) { std::lock_guard lock(*mutex_); + setStateInternal(state); +} + +void +StateModel::setStateInternal(unsigned int state) { if (state != END_ST && !states_.isDefined(state)) { isc_throw(StateModelError, "Attempt to set state to an undefined value: " << state ); @@ -296,13 +312,19 @@ StateModel::setState(unsigned int state) { // If we're entering the new state we need to see if we should // pause the state model in this state. - if (on_entry_flag_ && !paused_ && (getState(state)->shouldPause())) { + if (on_entry_flag_ && !paused_ && getStateInternal(state)->shouldPause()) { paused_ = true; } } void StateModel::postNextEvent(unsigned int event_value) { + std::lock_guard lock(*mutex_); + postNextEventInternal(event_value); +} + +void +StateModel::postNextEventInternal(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)) { @@ -310,13 +332,13 @@ StateModel::postNextEvent(unsigned int event_value) { "Attempt to post an undefined event, value: " << event_value); } - std::lock_guard lock(*mutex_); last_event_ = next_event_; next_event_ = event_value; } bool StateModel::doOnEntry() { + std::lock_guard lock(*mutex_); bool ret = on_entry_flag_; on_entry_flag_ = false; return (ret); @@ -324,6 +346,7 @@ StateModel::doOnEntry() { bool StateModel::doOnExit() { + std::lock_guard lock(*mutex_); bool ret = on_exit_flag_; on_exit_flag_ = false; return (ret); @@ -352,9 +375,15 @@ StateModel::getNextEvent() const { std::lock_guard lock(*mutex_); return (next_event_); } + bool StateModel::isModelNew() const { std::lock_guard lock(*mutex_); + return isModelNewInternal(); +} + +bool +StateModel::isModelNewInternal() const { return (curr_state_ == NEW_ST); } @@ -391,11 +420,23 @@ StateModel::isModelPaused() const { std::string StateModel::getStateLabel(const int state) const { + std::lock_guard lock(*mutex_); + return getStateLabelInternal(state); +} + +std::string +StateModel::getStateLabelInternal(const int state) const { return (states_.getLabel(state)); } std::string StateModel::getEventLabel(const int event) const { + std::lock_guard lock(*mutex_); + return getEventLabelInternal(event); +} + +std::string +StateModel::getEventLabelInternal(const int event) const { return (events_.getLabel(event)); } @@ -404,10 +445,10 @@ StateModel::getContextStr() const { std::lock_guard lock(*mutex_); std::ostringstream stream; stream << "current state: [ " - << curr_state_ << " " << getStateLabel(curr_state_) + << curr_state_ << " " << getStateLabelInternal(curr_state_) << " ] next event: [ " - << next_event_ << " " << getEventLabel(next_event_) << " ]"; - return(stream.str()); + << next_event_ << " " << getEventLabelInternal(next_event_) << " ]"; + return (stream.str()); } std::string @@ -415,10 +456,10 @@ StateModel::getPrevContextStr() const { std::lock_guard lock(*mutex_); std::ostringstream stream; stream << "previous state: [ " - << prev_state_ << " " << getStateLabel(prev_state_) + << prev_state_ << " " << getStateLabelInternal(prev_state_) << " ] last event: [ " - << next_event_ << " " << getEventLabel(last_event_) << " ]"; - return(stream.str()); + << next_event_ << " " << getEventLabelInternal(last_event_) << " ]"; + return (stream.str()); } } // namespace isc::util diff --git a/src/lib/util/state_model.h b/src/lib/util/state_model.h index 7f487b88de..3df406dbaa 100644 --- a/src/lib/util/state_model.h +++ b/src/lib/util/state_model.h @@ -364,6 +364,7 @@ public: void nopStateHandler(); protected: + /// @brief Initializes the event and state dictionaries. /// /// This method invokes the define and verify methods for both events and @@ -394,10 +395,15 @@ protected: /// : /// } /// @endcode + /// + /// This method is called in a thread safe context from + /// @ref initDictionaries. virtual void defineEvents(); /// @brief Adds an event value and associated label to the set of events. /// + /// This method is called in a thread safe context from @ref defineEvents. + /// /// @param value is the numeric value of the event /// @param label is the text label of the event used in log messages and /// exceptions. @@ -408,6 +414,8 @@ protected: /// @brief Fetches the event referred to by value. /// + /// This method is called in a thread safe context from @ref verifyEvents. + /// /// @param value is the numeric value of the event desired. /// /// @return returns a constant pointer reference to the event if found @@ -438,6 +446,9 @@ protected: /// : /// } /// @endcode + /// + /// This method is called in a thread safe context from + /// @ref initDictionaries. virtual void verifyEvents(); /// @brief Populates the set of states. @@ -461,10 +472,15 @@ protected: /// : /// } /// @endcode + /// + /// This method is called in a thread safe context from + /// @ref initDictionaries. virtual void defineStates(); /// @brief Adds an state value and associated label to the set of states. /// + /// This method is called in a thread safe context from @ref defineStates. + /// /// @param value is the numeric value of the state /// @param label is the text label of the state used in log messages and /// exceptions. @@ -510,6 +526,9 @@ protected: /// : /// } /// @endcode + /// + /// This method is called in a thread safe context from + /// @ref initDictionaries. virtual void verifyStates(); /// @brief Handler for fatal model execution errors. @@ -595,6 +614,7 @@ protected: bool doOnExit(); public: + /// @brief Fetches the model's current state. /// /// This returns the model's notion of the current state. It is the @@ -692,7 +712,98 @@ public: /// @return Returns a std::string of the format described above. std::string getPrevContextStr() const; +protected: + + /// @brief Fetches the state referred to by value. + /// + /// This method should be called in a thread safe context. + /// + /// @param value is the numeric value of the state desired. + /// + /// @return returns a constant pointer to the state if found + /// + /// @throw StateModelError if the state is not defined. + const StatePtr getStateInternal(unsigned int value); + private: + + /// @brief Sets the current state to the given state value. + /// + /// This updates the model's notion of the current state and is the + /// state whose handler will be executed on the next iteration of the run + /// loop. This is intended primarily for internal use and testing. It is + /// unlikely that transitioning to a new state without a new event is of + /// much use. + /// This method should be called in a thread safe context. + /// + /// @param state the new value to assign to the current state. + /// + /// @throw StateModelError if the state is invalid. + void setStateInternal(unsigned int state); + + /// @brief Sets the next event to the given event value. + /// + /// This updates the model's notion of the next event and is the + /// event that will be passed into the current state's handler on the next + /// iteration of the run loop. + /// This method should be called in a thread safe context. + /// + /// @param event the numeric event value to post as the next event. + /// + /// @throw StateModelError if the event is undefined + void postNextEventInternal(unsigned int event); + + /// @brief Returns whether or not the model is new. + /// + /// This method should be called in a thread safe context. + /// + /// @return Boolean true if the model has not been started. + bool isModelNewInternal() const; + + /// @brief Fetches the label associated with an event value. + /// + /// This method should be called in a thread safe context. + /// + /// @param event is the numeric event value for which the label is desired. + /// + /// @return Returns a string containing the event label or + /// LabeledValueSet::UNDEFINED_LABEL if the value is undefined. + std::string getEventLabelInternal(const int event) const; + + /// @brief Fetches the label associated with an state value. + /// + /// This method should be called in a thread safe context. + /// + /// @param state is the numeric state value for which the label is desired. + /// + /// @return Returns a const char* containing the state label or + /// LabeledValueSet::UNDEFINED_LABEL if the value is undefined. + std::string getStateLabelInternal(const int state) const; + + /// @brief Convenience method which returns a string rendition of the + /// current state and next event. + /// + /// The string will be of the form: + /// + /// current state: [ {state} {label} ] next event: [ {event} {label} ] + /// + /// This method should be called in a thread safe context. + /// + /// @return Returns a std::string of the format described above. + std::string getContextStrInternal() const; + + /// @brief Convenience method which returns a string rendition of the + /// previous state and last event. + /// + /// The string will be of the form: + /// + /// previous state: [ {state} {label} ] last event: [ {event} {label} ] + /// + /// This method should be called in a thread safe context. + /// + /// @return Returns a std::string of the format described above. + std::string getPrevContextStrInternal() const; + /// @brief The dictionary of valid events. LabeledValueSet events_; diff --git a/src/lib/util/tests/state_model_unittest.cc b/src/lib/util/tests/state_model_unittest.cc index 9906d44bc4..1ccf45a34f 100644 --- a/src/lib/util/tests/state_model_unittest.cc +++ b/src/lib/util/tests/state_model_unittest.cc @@ -234,12 +234,12 @@ public: StateModel::verifyStates(); // Verify our states. - getState(DUMMY_ST); - getState(READY_ST); - getState(DO_WORK_ST); - getState(DONE_ST); - getState(PAUSE_ALWAYS_ST); - getState(PAUSE_ONCE_ST); + getStateInternal(DUMMY_ST); + getStateInternal(READY_ST); + getStateInternal(DO_WORK_ST); + getStateInternal(DONE_ST); + getStateInternal(PAUSE_ALWAYS_ST); + getStateInternal(PAUSE_ONCE_ST); } /// @brief Manually construct the event and state dictionaries.