Hello, looks good to me. Got a question and a comment below under the [rafael] tag.
On 10/31/2016 09:31 PM, Alex Jones wrote: > osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc | 175 > +++++++++++++++++++++- > osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh | 3 + > osaf/services/saf/smfsv/smfd/SmfStepTypes.cc | 32 ++- > 3 files changed, 191 insertions(+), 19 deletions(-) > > > This patch adds support for section 4.2.1.3 of SMF A.01.02 spec. > > diff --git a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc > b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc > --- a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc > +++ b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc > @@ -39,6 +39,7 @@ > #include "SmfUtils.hh" > > SmfCampaignThread *SmfCampaignThread::s_instance = NULL; > +SaNtfSubscriptionIdT SmfCampaignThread::operStateSubId(1); > > #define SMF_RDA_RETRY_COUNT 25 /* This is taken as the csi's are assigned > in parallel */ > > @@ -140,7 +141,12 @@ void SmfCampaignThread::main(NCSCONTEXT > SmfCampaignThread::~SmfCampaignThread() > { > TRACE_ENTER(); > - SaAisErrorT rc = saNtfFinalize(m_ntfHandle); > + SaAisErrorT rc(saNtfNotificationUnsubscribe(operStateSubId)); > + if (rc != SA_AIS_OK) { > + LOG_ER("Failed to unsubscribe to oper state notifications %u", > rc); > + } > + > + rc = saNtfFinalize(m_ntfHandle); > if (rc != SA_AIS_OK) { > LOG_ER("Failed to finalize NTF handle %u", rc); > } > @@ -303,10 +309,14 @@ int SmfCampaignThread::initNtf(void) > { > SaAisErrorT rc = SA_AIS_ERR_TRY_AGAIN; > SaVersionT ntfVersion = { 'A', 1, 1 }; > + SaNtfCallbacksT callbacks = { > + ntfNotificationCallback, > + 0 > + }; > unsigned int numOfTries = 50; > > while (rc == SA_AIS_ERR_TRY_AGAIN && numOfTries > 0) { > - rc = saNtfInitialize(&m_ntfHandle, NULL, &ntfVersion); > + rc = saNtfInitialize(&m_ntfHandle, &callbacks, &ntfVersion); > if (rc != SA_AIS_ERR_TRY_AGAIN) { > break; > } > @@ -319,9 +329,142 @@ int SmfCampaignThread::initNtf(void) > return -1; > } > > + // subscribe to operational state change notifications generated by AMF > + rc = initNtfSubscriptions(); > + if (rc != SA_AIS_OK) { > + LOG_ER("initNtfSubscriptions FAILED rc=%s", saf_error(rc)); > + return -1; > + } > + > return 0; > } > > +SaAisErrorT SmfCampaignThread::initNtfSubscriptions(void) { > + TRACE_ENTER(); > + SaAisErrorT rc(SA_AIS_OK); > + > + do { > + SaNtfStateChangeNotificationFilterT stateChangeFilter; > + > + SaAisErrorT rc(saNtfStateChangeNotificationFilterAllocate( > + m_ntfHandle, > + &stateChangeFilter, > + 0, > + 0, > + 0, > + 0, > + 0, > + 0)); > + > + if (rc != SA_AIS_OK) { > + LOG_ER("saNtfAttributeChangeNotificationFilterAllocate FAILED rc=%s", > + saf_error(rc)); > + break; > + } > + > + SaNtfNotificationTypeFilterHandlesT notificationFilterHandles = { > + 0, > + 0, > + stateChangeFilter.notificationFilterHandle, > + 0, > + 0 > + }; > + > + rc = saNtfNotificationSubscribe(¬ificationFilterHandles, > operStateSubId); > + > + if (rc != SA_AIS_OK) { > + LOG_ER("saNtfNotificationSubscribe FAILED rc=%s", saf_error(rc)); > + break; > + } > + > + rc = > saNtfNotificationFilterFree(stateChangeFilter.notificationFilterHandle); > + if (rc != SA_AIS_OK) { > + LOG_ER("saNtfNotificationFilterFree FAILED rc=%s", saf_error(rc)); > + break; > + } > + } while (false); > + > + TRACE_LEAVE(); > + return rc; > +} > + > +void SmfCampaignThread::ntfNotificationCallback( > + SaNtfSubscriptionIdT subId, > + const SaNtfNotificationsT *notification) { > + TRACE_ENTER(); > + > + do { > + if (subId != operStateSubId) { > + TRACE("unknown subscription id received in ntfNotificationCallback: > %d", subId); > + break; > + } > + > + // check if an SU was disabled after it had been upgraded > + if (notification->notificationType == SA_NTF_TYPE_STATE_CHANGE) { > + const SaNtfStateChangeNotificationT& stateChangeNotification( > + notification->notification.stateChangeNotification); > + > + if (stateChangeNotification.notificationHeader.eventType && > + *stateChangeNotification.notificationHeader.eventType == > + SA_NTF_OBJECT_STATE_CHANGE) { > + if (stateChangeNotification.notificationHeader.notificationClassId && > + > stateChangeNotification.notificationHeader.notificationClassId->vendorId == > + SA_NTF_VENDOR_ID_SAF && > + > stateChangeNotification.notificationHeader.notificationClassId->majorId == > + SA_SVC_AMF && > + > stateChangeNotification.notificationHeader.notificationClassId->minorId == > + SA_AMF_NTFID_SU_OP_STATE) { [rafael] If possible try to improve the readability. This would be applicable here. Specially for the do while and nested if clauses. http://www.informit.com/articles/article.aspx?p=1398607 > + for (SaUint16T i(0); i < stateChangeNotification.numStateChanges; > i++) { > + if (stateChangeNotification.changedStates[i].newState == > + SA_AMF_OPERATIONAL_DISABLED && > + stateChangeNotification.notificationHeader.additionalInfo) { > + for (SaUint16T j(0); > + j < > stateChangeNotification.notificationHeader.numAdditionalInfo; > + j++) { > + if > (stateChangeNotification.notificationHeader.additionalInfo[j].infoId > + == SA_AMF_MAINTENANCE_CAMPAIGN_DN && > + > stateChangeNotification.notificationHeader.additionalInfo[j].infoType > + == SA_NTF_VALUE_LDAP_NAME) { > + SaNameT *name(0); > + SaUint16T dataSize(0); > + > + SaAisErrorT rc(saNtfPtrValGet( > + stateChangeNotification.notificationHandle, > + > &stateChangeNotification.notificationHeader.additionalInfo[j].infoValue, > + reinterpret_cast<void **>(&name), > + &dataSize)); > + > + if (rc == SA_AIS_OK) { > + SaConstStringT > maintenanceCampaign(saAisNameBorrow(name)); > + > + if (!strcmp(maintenanceCampaign, > + s_instance->m_campaign->getDn().c_str())) { > + LOG_ER("SU: %s failed after upgrade in campaign", > + > saAisNameBorrow(stateChangeNotification.notificationHeader.notificationObject)); > + > s_instance->m_campaign->getUpgradeCampaign()->suspend(); > + > s_instance->m_campaign->getUpgradeCampaign()->setCampState( > + SA_SMF_CMPG_ERROR_DETECTED); > + > + std::string error(saAisNameBorrow( > + > stateChangeNotification.notificationHeader.notificationObject)); > + > + error += " failed after upgrade"; > + s_instance->m_campaign->setError(error); > + } > + } else { > + LOG_ER("saNtfPtrValGet failed: %d", rc); > + } > + } > + } > + } > + } > + } > + } > + } > + } while (false); > + > + TRACE_LEAVE(); > +} > /** > * SmfCampaignThread::send > * send event to the thread. > @@ -672,18 +815,31 @@ int SmfCampaignThread::handleEvents(void > { > TRACE_ENTER(); > NCS_SEL_OBJ mbx_fd = ncs_ipc_get_sel_obj(&m_mbx); > - struct pollfd fds[1]; > + > + SaSelectionObjectT ntfSelObj(0); > + > + SaAisErrorT rc(saNtfSelectionObjectGet(m_ntfHandle, &ntfSelObj)); > + if (rc != SA_AIS_OK) { > + LOG_ER("saNtfSelectionObjectGet FAILED - %s", saf_error(rc)); > + return 1; > + } > + > + struct pollfd fds[2]; > > /* Set up all file descriptors to listen to */ > fds[0].fd = mbx_fd.rmv_obj; > fds[0].events = POLLIN; > fds[0].revents = 0; > > + fds[1].fd = ntfSelObj; > + fds[1].events = POLLIN; > + fds[1].revents = 0; > + > TRACE("Campaign thread %s waiting for events", > m_campaign->getDn().c_str()); > > while (m_running) { > > - int ret = poll(fds, 1, SMF_UPDATE_ELAPSED_TIME_INTERVAL); > + int ret = poll(fds, sizeof(fds) / sizeof(pollfd), > SMF_UPDATE_ELAPSED_TIME_INTERVAL); > > if (ret == -1) { > if (errno == EINTR) > @@ -699,7 +855,16 @@ int SmfCampaignThread::handleEvents(void > processEvt(); > } > > - m_campaign->updateElapsedTime(); > + if (fds[1].revents & POLLIN) { > + // dispatch NTF events > + rc = saNtfDispatch(m_ntfHandle, SA_DISPATCH_ALL); > + > + if (rc != SA_AIS_OK) { > + LOG_ER("saNtfDispatch FAILED - %s", saf_error(rc)); > + } > + } > + > + m_campaign->updateElapsedTime(); > } > TRACE_LEAVE(); > return 0; > diff --git a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh > b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh > --- a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh > +++ b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh > @@ -190,9 +190,12 @@ class SmfCampaignThread { > > SaAisErrorT createImmHandle(SmfCampaign * i_campaign); > SaAisErrorT deleteImmHandle(); > + SaAisErrorT initNtfSubscriptions(void); > > + static void ntfNotificationCallback(SaNtfSubscriptionIdT, const > SaNtfNotificationsT *); > static void main(NCSCONTEXT info); > static SmfCampaignThread *s_instance; > + static SaNtfSubscriptionIdT operStateSubId; > > NCSCONTEXT m_task_hdl; > SYSF_MBX m_mbx; /* mailbox */ > diff --git a/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc > b/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc > --- a/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc > +++ b/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc > @@ -1247,8 +1247,8 @@ SmfStepTypeNodeReboot::execute() > return false; > } > > - /* Modify information model and set maintenance status */ > - LOG_NO("STEP: Modify information model and set maintenance status"); > + /* Modify information model */ > + LOG_NO("STEP: Modify information model"); > if (m_step->modifyInformationModel() != SA_AIS_OK) { > LOG_ER("Failed to Modify information model in > step=%s",m_step->getRdn().c_str()); > return false; > @@ -1261,11 +1261,6 @@ SmfStepTypeNodeReboot::execute() > return false; > } > > - if (m_step->setMaintenanceStateActUnits() == false) { > - LOG_ER("Failed to set maintenance state in > step=%s",m_step->getRdn().c_str()); > - return false; > - } > - > /* The action below is an add on to > SMF.---------------------------------------*/ > /* See if any of the software bundles installed at online > installation has the */ > /* saSmfBundleInstallOfflineScope attribute set to > SA_SMF_CMD_SCOPE_PLM_EE */ > @@ -1372,6 +1367,13 @@ SmfStepTypeNodeReboot::execute() > /* the units in the same state as before locking > */ > m_step->copyDuInitStateToAu(); > > + /* Now that the node is up, set the maintenance status */ > + LOG_NO("STEP: Set Maintenance Status"); > + if (m_step->setMaintenanceStateActUnits() == false) { > + LOG_ER("Failed to set maintenance state in > step=%s",m_step->getRdn().c_str()); > + return false; > + } > + [rafael] Why was setting the maintenance status moved to after the node up? > /* Instantiate activation units */ > LOG_NO("STEP: Instantiate activation units"); > if (m_step->instantiateActivationUnits() == false) { > @@ -1685,8 +1687,8 @@ SmfStepTypeNodeRebootAct::execute() > return false; > } > > - /* Modify information model and set maintenance status */ > - LOG_NO("STEP: Modify information model and set maintenance status"); > + /* Modify information model */ > + LOG_NO("STEP: Modify information model"); > if (m_step->modifyInformationModel() != SA_AIS_OK) { > LOG_ER("Failed to Modify information model in > step=%s",m_step->getRdn().c_str()); > return false; > @@ -1699,11 +1701,6 @@ SmfStepTypeNodeRebootAct::execute() > return false; > } > > - if (m_step->setMaintenanceStateActUnits() == false) { > - LOG_ER("Failed to set maintenance state in > step=%s",m_step->getRdn().c_str()); > - return false; > - } > - > /* Offline installation of new software */ > LOG_NO("STEP: Offline installation of new software"); > if (m_step->offlineInstallNewBundles() == false) { > @@ -1746,6 +1743,13 @@ SmfStepTypeNodeRebootAct::execute() > /* the units in the same state as before locking > */ > m_step->copyDuInitStateToAu(); > > + /* Now that the node is up, set the maintenance status */ > + LOG_NO("STEP: Set Maintenance Status"); > + if (m_step->setMaintenanceStateActUnits() == false) { > + LOG_ER("Failed to set maintenance state in > step=%s",m_step->getRdn().c_str()); > + return false; > + } > + > /* Instantiate activation units */ > LOG_NO("STEP: Instantiate activation units"); > if (m_step->instantiateActivationUnits() == false) { > ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel