[devel] [PATCH 1 of 1] amfnd: send node_up even if led_state is not green

[Gary Lee]

 osaf/services/saf/amf/amfnd/di.cc |  5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)


if amfd restarts before set_leds is received by amfnd, then
amfnd will not send node_up. amfnd should send node_up
even if led state is not green.

diff --git a/osaf/services/saf/amf/amfnd/di.cc 
b/osaf/services/saf/amf/amfnd/di.cc
--- a/osaf/services/saf/amf/amfnd/di.cc
+++ b/osaf/services/saf/amf/amfnd/di.cc
@@ -562,13 +562,12 @@ uint32_t avnd_evt_mds_avd_up_evh(AVND_CB
 */
if (evt->info.mds.i_change == NCSMDS_NEW_ACTIVE && 
cb->is_avd_down) {
if (cb->led_state == AVND_LED_STATE_GREEN) {
-   LOG_NO("Sending node up due to 
NCSMDS_NEW_ACTIVE");
-
// node_up, sync sisu, compcsi info to AVND for 
recovery
avnd_sync_sisu(cb);
avnd_sync_csicomp(cb);
-   avnd_send_node_up_msg();
}
+   LOG_NO("Sending node up due to NCSMDS_NEW_ACTIVE");
+   avnd_send_node_up_msg();
}
cb->is_avd_down = 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

[devel] [PATCH 0 of 1] Review Request for amfnd: send node_up even if led_state is not green [#2183]

[Gary Lee]

Summary: amfnd: send node_up even if led_state is not green 
Review request for Trac Ticket(s): 2183 
Peer Reviewer(s): AMF devs 
Pull request to: <>
Affected branch(es): all 
Development branch: default 


Impacted area   Impact y/n

 Docsn
 Build systemn
 RPM/packaging   n
 Configuration files n
 Startup scripts n
 SAF servicesy 
 OpenSAF servicesn
 Core libraries  n
 Samples n
 Tests   n
 Other   n


Comments (indicate scope for each "y" above):
-

changeset 891d9f82a5028ffc7cdbe1fb18e3de696bb2b9b1
Author: Gary Lee 
Date:   Fri, 11 Nov 2016 16:40:42 +1100

amfnd: send node_up even if led_state is not green

if amfd restarts before set_leds is received by amfnd, then amfnd will 
not
send node_up. amfnd should send node_up even if led state is not green.


Complete diffstat:
--
 osaf/services/saf/amf/amfnd/di.cc |  5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)


Testing Commands:
-
see ticket

Testing, Expected Results:
--
amfnd sends node_up

Conditions of Submission:
-


Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y 
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.hgrc file (i.e. username, email etc)

___ Your computer have a badly configured date and time; confusing the
the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results
for in-service upgradability test.

___ Your changes affect user manual and documentation, your patch series
do not contain the patch that updates the Doxygen manual.


--
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

Re: [devel] [PATCH 1 of 1] smf: Fails to create a node group, admin owner/handle is lost [#2153]

[Neelakanta Reddy]

Hi Lennart,

comments inline

On 2016/11/07 05:42 PM, Lennart Lund wrote:
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  46 
> +++--
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.hh |   1 +
>   2 files changed, 36 insertions(+), 11 deletions(-)
>
>
> Recreate handles and admin owner if creating a node group fail
>
> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc 
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> @@ -2816,6 +2816,17 @@ SmfAdminOperation::SmfAdminOperation(std
>   m_smfKeepDuState = true;
>   }
>   
> + // Create an instance unique name for a node group IMM object
> + // using an instance number.
> + m_instance_number = m_next_instance_number++;
> + m_instanceNodeGroupName = "safAmfNodeGroup=smfLockAdmNg"+
> + std::to_string(m_instance_number);
> + m_admin_owner_name ="smfNgAdmOwnerName";
> + TRACE("%s m_instanceNodeGroupName '%s'",__FUNCTION__,
> + m_instanceNodeGroupName.c_str());
> + TRACE("%s m_NodeGroupAdminOwnerName '%s'", __FUNCTION__,
> + m_admin_owner_name.c_str());
> +
>   // Note:
>   // If any of the following steps fails m_creation_fail is set to true
>   // and an immediate return is done
> @@ -2835,12 +2846,6 @@ SmfAdminOperation::SmfAdminOperation(std
>   m_creation_fail = true;
>   return;
>   }
> -
> - // Create an instance unique name for a node group IMM object using an
> - // instance number.
> - m_instance_number = m_next_instance_number++;
> - m_instanceNodeGroupName = "safAmfNodeGroup=smfLockAdmNg"+
> - std::to_string(m_instance_number);
>   }
>   
>   SmfAdminOperation::~SmfAdminOperation()
> @@ -3103,7 +3108,8 @@ bool SmfAdminOperation::initNodeGroupOm(
>   timeout_try_cnt = 6;
>   while (timeout_try_cnt > 0) {
>   ais_rc = immutil_saImmOmAdminOwnerInitialize(m_omHandle,
> - const_cast ("smfSetAdminStateOwner"),
> + const_cast
> + (m_admin_owner_name.c_str()),
>   SA_TRUE, _ownerHandle);
>   if (ais_rc != SA_AIS_ERR_TIMEOUT)
>   break;
> @@ -3162,7 +3168,8 @@ bool SmfAdminOperation::becomeAdminOwner
>   objectNames, SA_IMM_SUBTREE);
>   bool rc = true;
>   if (ais_rc != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmAdminOwnerSet Fail '%s'", __FUNCTION__,
> + LOG_NO("%s: saImmOmAdminOwnerSet owner name '%s' Fail '%s'",
> + __FUNCTION__, m_admin_owner_name.c_str(),
>   saf_error(ais_rc));
>   }
>   
> @@ -3470,7 +3477,9 @@ bool SmfAdminOperation::createNodeGroup(
>   {
>   TRACE_ENTER();
>   
> - TRACE("\t uniqueNodeName '%s'", m_instanceNodeGroupName.c_str());
> + TRACE("\t unique Node name '%s'", m_instanceNodeGroupName.c_str());
> + TRACE("\t unique Admin owner name '%s'",
> + m_admin_owner_name.c_str());
>   
>   // 
>   // Attribute descriptor for the node name
> @@ -3556,7 +3565,7 @@ bool SmfAdminOperation::createNodeGroup(
>   // Create the node group
>   m_errno = SA_AIS_OK;
>   bool method_rc = false;
> -const uint32_t MAX_NO_RETRIES = 2;
> +const uint32_t MAX_NO_RETRIES = 4;
>   uint32_t retry_cnt = 0;
>   while (++retry_cnt <= MAX_NO_RETRIES) {
>   // Creating the node group object will fail if a previously
> @@ -3585,7 +3594,22 @@ bool SmfAdminOperation::createNodeGroup(
>   break;
>   }
>   continue;
> -} else if (m_errno != SA_AIS_OK) {
> +} else if (m_errno == SA_AIS_ERR_BAD_HANDLE ||
> +m_errno == SA_AIS_ERR_BAD_OPERATION) {
What is the reason for retrying fro BAD_OPERATION?

Thanks,
Neel.

> +LOG_NO("%s: saImmOmCcbObjectCreate_2 Fail %s",
> +   __FUNCTION__, saf_error(m_errno));
> +finalizeNodeGroupOm();
> +bool rc = initNodeGroupOm();
> +if (rc == false) {
> +LOG_NO("%s: initNodeGroupOm() Fail",
> +   __FUNCTION__);
> +method_rc = false;
> +break;
> +}
> + TRACE("\t Try again after %s",
> + saf_error(m_errno));
> +continue;
> + } else if (m_errno != SA_AIS_OK) {
>   LOG_NO("%s: 

Re: [devel] [PATCH 1 of 1] amfd: execute imm jobs based on latest status of attributes and objects[#2009] V2

[Gary Lee]

Hi Praveen

Ack with one comment below (review only)

Thanks
Gary

> On 10 Nov. 2016, at 6:13 pm, praveen.malv...@oracle.com wrote:
> 
> osaf/services/saf/amf/amfd/ckpt_updt.cc   |4 +
> osaf/services/saf/amf/amfd/imm.cc |  386 
> +-
> osaf/services/saf/amf/amfd/include/imm.h  |   22 +-
> osaf/services/saf/amf/amfd/include/util.h |2 +
> osaf/services/saf/amf/amfd/siass.cc   |7 +-
> osaf/services/saf/amf/amfd/util.cc|   28 ++
> 6 files changed, 438 insertions(+), 11 deletions(-)
> 
> 
> V2:Incorporated comments given by Gary.
> 
> App Si is moving to UNASSIGNED state after middleware failover.
> 
> Standby controller maintains a job queue for SU, SISU and COMPCSI.
> In job queue both old and new state of a object or attribute will be present.
> The issue is not observed normally because standby will update correct
> state eventually from old to new. In the cases mentioned in the ticket:
> -in one case, SI was deleted. Because of this after failover, IMM returns 
> failure for creating the SISU because
>  this new active AMFD is executing old job.
> -in other case, Su was deleted and was added with new state. Here also after 
> failover new active AMFD updates
>  IMM with old state as it is executing old job.
> 
> Patch fixes the problem by:
> -remebering if job was created in active or stadby state of AMFD.
> -After failover if new active finds some job that was created in standby 
> state, it checks
>  the status of objects or attributes and based on that job will be executed 
> with latest values or simple deleted.
> 
> diff --git a/osaf/services/saf/amf/amfd/ckpt_updt.cc 
> b/osaf/services/saf/amf/amfd/ckpt_updt.cc
> --- a/osaf/services/saf/amf/amfd/ckpt_updt.cc
> +++ b/osaf/services/saf/amf/amfd/ckpt_updt.cc
> @@ -463,6 +463,10 @@ uint32_t avd_ckpt_siass(AVD_CL_CB *cb, A
>   } else {
>   su_si_rel_ptr->comp_name = "";
>   su_si_rel_ptr->csi_name = "";
> + //This is update of SUSI because of 
> modification of assignment. 
> + if (action == NCS_MBCSV_ACT_UPDATE) {
> + avd_susi_update(su_si_rel_ptr, 
> su_si_rel_ptr->state);
> + }
>   }
>   } else {
>   LOG_ER("%s:%u", __FUNCTION__, __LINE__);
> diff --git a/osaf/services/saf/amf/amfd/imm.cc 
> b/osaf/services/saf/amf/amfd/imm.cc
> --- a/osaf/services/saf/amf/amfd/imm.cc
> +++ b/osaf/services/saf/amf/amfd/imm.cc
> @@ -142,6 +142,12 @@ AvdJobDequeueResultT ImmObjCreate::exec(
>   TRACE_ENTER2("Create %s", parentName_.c_str());
> 
>   const SaNameTWrapper parent_name(parentName_);
> + //Check in AMF Db before creation in IMM.
> + if (immobj_update_required() == false){
> + delete Fifo::dequeue();
> + res = JOB_EXECUTED;
> + goto done;  
> + }
>   rc = saImmOiRtObjectCreate_2(immOiHandle, className_,
>parent_name, attrValues_);
> 
> @@ -163,7 +169,7 @@ AvdJobDequeueResultT ImmObjCreate::exec(
>   LOG_ER("%s: create FAILED %u", __FUNCTION__, rc);
>   res = JOB_ERR;
>   }
> - 
> +done:
>   TRACE_LEAVE();
>   
>   return res;
> @@ -211,6 +217,12 @@ AvdJobDequeueResultT ImmObjUpdate::exec(
> 
>   TRACE_ENTER2("Update '%s' %s", dn.c_str(), attributeName_);
> 
> + //update latest values.
> + if (immobj_update_required() == false){
> + delete Fifo::dequeue();
> + res = JOB_EXECUTED;
> + goto done;  
> + }
>   attrMod.modType = SA_IMM_ATTR_VALUES_REPLACE;
>   attrMod.modAttr.attrName = attributeName_;
>   attrMod.modAttr.attrValuesNumber = 1;
> @@ -237,7 +249,7 @@ AvdJobDequeueResultT ImmObjUpdate::exec(
>   LOG_ER("%s: update FAILED %u", __FUNCTION__, rc);
>   res = JOB_ERR;
>   }
> - 
> +done:
>   TRACE_LEAVE();
>   return res;
> }
> @@ -260,6 +272,12 @@ AvdJobDequeueResultT ImmObjDelete::exec(
> 
>   TRACE_ENTER2("Delete %s", dn.c_str());
> 
> + //Check AMF db before deletion.
> + if (immobj_update_required() == false){
> + delete Fifo::dequeue();
> + res = JOB_EXECUTED;
> + goto done;  
> + }
>   rc = saImmOiRtObjectDelete_o3(immOiHandle, dn.c_str());
> 
>   if ((rc == SA_AIS_OK) || (rc == SA_AIS_ERR_NOT_EXIST)) {
> @@ -280,7 +298,7 @@ AvdJobDequeueResultT ImmObjDelete::exec(
>   LOG_ER("%s: delete FAILED %u", __FUNCTION__, rc);
>   res = JOB_ERR;
>   }
> - 
> +done:
>   TRACE_LEAVE();
>   return res;
> }
> @@ -478,8 +496,8 @@ static const char *avd_class_names[] = {
>   "SaAmfSIDependency",
>   "SaAmfSIRankedSU",
> 
> - "SaAmfCSIAssignment",
> - "SaAmfSIAssignment"
> + 

Re: [devel] [PATCH 1 of 1] smfd: add support for detection of asynchronous failures of AMF entities [#2145]

[Alex Jones]

Thanks for the comments Rafael.

1) I will look at that page, and refactor that part of the code. I agree
that it is difficult to read.

2) The reason setting the maintenance status was moved to after the node
up, is because AMF will generate the operational state change
notification with the saAmfSUMaintenanceCampaign object set in the
notification for the old software when the node is brought down. SMF
would detect this as a failure, even though it is not, because it is the
old software going down. It was being set too early. According to
4.2.1.3 it should be set right before the activation unit is activated
with the new software.

I will submit a new patch soon.

Alex

On 11/10/2016 05:17 AM, Rafael Odzakow wrote:
> 
> NOTICE: This email was received from an EXTERNAL sender
> 
> 
> 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(_ntfHandle, NULL, );
>> + rc = saNtfInitialize(_ntfHandle, , );
>> 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,
>> + ,
>> + 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(,
> 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 &&
>> + 

Re: [devel] [PATCH 0 of 1] Review Request for smf: Fails to create a node group, admin owner/handle is lost [#2153]

[Rafael Odzakow]

ACK


On 11/07/2016 01:12 PM, Lennart Lund wrote:
> Summary: smf: Fails to create a node group, admin owner/handle is lost
> Review request for Trac Ticket(s): #2153
> Peer Reviewer(s): rafael.odza...@ericsson.com, reddy.neelaka...@oracle.com
> Pull request to:
> Affected branch(es): 5.1 ->
> Development branch: <>
>
> 
> Impacted area   Impact y/n
> 
>   Docsn
>   Build systemn
>   RPM/packaging   n
>   Configuration files n
>   Startup scripts n
>   SAF servicesy
>   OpenSAF servicesn
>   Core libraries  n
>   Samples n
>   Tests   n
>   Other   n
>
>
> Comments (indicate scope for each "y" above):
> -
>   <>
>
> changeset c693485eac53ec9792c067f9815fe93f99c5baa3
> Author:   Lennart Lund 
> Date: Fri, 04 Nov 2016 13:42:29 +0100
>
>   smf: Fails to create a node group, admin owner/handle is lost [#2153]
>
>   Recreate handles and admin owner if creating a node group fail
>
>
> Complete diffstat:
> --
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  46 
> +++---
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.hh |   1 +
>   2 files changed, 36 insertions(+), 11 deletions(-)
>
>
> Testing Commands:
> -
> Intermittent problem. Campaign with nodes as deacivation/activation units
> may fail because of BAD HANDLE or BAD OPERATION when creating a node group.
> Run such a campaign many times
>
>
> Testing, Expected Results:
> --
> Sahll never fail as described above
>
>
> Conditions of Submission:
> -
> Will be pushed after one week or ack from all reviewers
>
>
> Arch  Built StartedLinux distro
> ---
> mipsn  n
> mips64  n  n
> x86 n  n
> x86_64  n  n
> powerpc n  n
> powerpc64   n  n
>
>
> Reviewer Checklist:
> ---
> [Submitters: make sure that your review doesn't trigger any checkmarks!]
>
>
> Your checkin has not passed review because (see checked entries):
>
> ___ Your RR template is generally incomplete; it has too many blank entries
>  that need proper data filled in.
>
> ___ You have failed to nominate the proper persons for review and push.
>
> ___ Your patches do not have proper short+long header
>
> ___ You have grammar/spelling in your header that is unacceptable.
>
> ___ You have exceeded a sensible line length in your headers/comments/text.
>
> ___ You have failed to put in a proper Trac Ticket # into your commits.
>
> ___ You have incorrectly put/left internal data in your comments/files
>  (i.e. internal bug tracking tool IDs, product names etc)
>
> ___ You have not given any evidence of testing beyond basic build tests.
>  Demonstrate some level of runtime or other sanity testing.
>
> ___ You have ^M present in some of your files. These have to be removed.
>
> ___ You have needlessly changed whitespace or added whitespace crimes
>  like trailing spaces, or spaces before tabs.
>
> ___ You have mixed real technical changes with whitespace and other
>  cosmetic code cleanup changes. These have to be separate commits.
>
> ___ You need to refactor your submission into logical chunks; there is
>  too much content into a single commit.
>
> ___ You have extraneous garbage in your review (merge commits etc)
>
> ___ You have giant attachments which should never have been sent;
>  Instead you should place your content in a public tree to be pulled.
>
> ___ You have too many commits attached to an e-mail; resend as threaded
>  commits, or place in a public tree for a pull.
>
> ___ You have resent this content multiple times without a clear indication
>  of what has changed between each re-send.
>
> ___ You have failed to adequately and individually address all of the
>  comments and change requests that were proposed in the initial review.
>
> ___ You have a misconfigured ~/.hgrc file (i.e. username, email etc)
>
> ___ Your computer have a badly configured date and time; confusing the
>  the threaded patch review.
>
> ___ Your changes affect IPC mechanism, and you don't present any results
>  for in-service upgradability test.
>
> ___ Your changes affect user manual and documentation, your patch series
>  do not contain the patch that updates the Doxygen manual.
>


--
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

[devel] [PATCH 0 of 1] Review Request for mds: Use the SOCK_CLOEXEC flag when creating sockets [#2181]

[Anders Widell]

Summary: mds: Use the SOCK_CLOEXEC flag when creating sockets [#2181]
Review request for Trac Ticket(s): 2181
Peer Reviewer(s): Mahesh
Pull request to: 
Affected branch(es): default(5.2)
Development branch: default


Impacted area   Impact y/n

 Docsn
 Build systemn
 RPM/packaging   n
 Configuration files n
 Startup scripts n
 SAF servicesn
 OpenSAF servicesn
 Core libraries  y
 Samples n
 Tests   n
 Other   n


Comments (indicate scope for each "y" above):
-

changeset 7483448ded099c28792d4ba8310b5c0589f61d72
Author: Anders Widell 
Date:   Thu, 10 Nov 2016 12:23:22 +0100

mds: Use the SOCK_CLOEXEC flag when creating sockets [#2181]

To avoid a potential race between fcntl(FD_CLOEXEC) in one thread and 
exec()
in another thread, use the SOCK_CLOEXEC flag when creating sockets.


Complete diffstat:
--
 osaf/libs/core/mds/mds_dt_tcp.c  |  23 ---
 osaf/libs/core/mds/mds_dt_tipc.c |  43 
+--
 2 files changed, 17 insertions(+), 49 deletions(-)


Testing Commands:
-
Run regression tests.


Testing, Expected Results:
--
Regression tests should pass.


Conditions of Submission:
-
Ack from reviewer(s).


Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.hgrc file (i.e. username, email etc)

___ Your computer have a badly configured date and time; confusing the
the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results
for in-service upgradability test.

___ Your changes affect user manual and documentation, your patch series
do not contain the patch that updates the Doxygen manual.


--
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

[devel] [PATCH 1 of 1] mds: Use the SOCK_CLOEXEC flag when creating sockets [#2181]

[Anders Widell]

 osaf/libs/core/mds/mds_dt_tcp.c  |  23 +++-
 osaf/libs/core/mds/mds_dt_tipc.c |  43 ---
 2 files changed, 17 insertions(+), 49 deletions(-)


To avoid a potential race between fcntl(FD_CLOEXEC) in one thread and exec() in
another thread, use the SOCK_CLOEXEC flag when creating sockets.

diff --git a/osaf/libs/core/mds/mds_dt_tcp.c b/osaf/libs/core/mds/mds_dt_tcp.c
--- a/osaf/libs/core/mds/mds_dt_tcp.c
+++ b/osaf/libs/core/mds/mds_dt_tcp.c
@@ -38,6 +38,13 @@
 #include 
 #include 
 
+#ifndef SOCK_CLOEXEC
+enum {
+   SOCK_CLOEXEC = 0x8
+};
+#define SOCK_CLOEXEC SOCK_CLOEXEC
+#endif
+
 #define MDS_MDTM_SUN_PATH 255
 #define MDS_MDTM_CONNECT_PATH PKGLOCALSTATEDIR "/osaf_dtm_intra_server"
 
@@ -81,7 +88,6 @@ uint32_t mdtm_process_recv_events_tcp(vo
  */
 uint32_t mds_mdtm_init_tcp(NODE_ID nodeid, uint32_t *mds_tcp_ref)
 {
-   uint32_t flags;
uint32_t sndbuf_size = 0; /* Send buffer size */
uint32_t rcvbuf_size = 0;  /* Receive buffer size */
socklen_t optlen; /* Option length */
@@ -125,7 +131,7 @@ uint32_t mds_mdtm_init_tcp(NODE_ID nodei
 
/* Create the sockets required for Binding, Send, receive and Discovery 
*/
 
-   tcp_cb->DBSRsock = socket(mds_socket_domain, SOCK_STREAM|SOCK_CLOEXEC, 
0);
+   tcp_cb->DBSRsock = socket(mds_socket_domain, SOCK_STREAM | 
SOCK_CLOEXEC, 0);
if (tcp_cb->DBSRsock < 0) {
syslog(LOG_ERR, "MDTM:TCP DBSRsock Socket creation failed in 
MDTM_INIT err :%s", strerror(errno));
return NCSCC_RC_FAILURE;
@@ -177,19 +183,6 @@ uint32_t mds_mdtm_init_tcp(NODE_ID nodei
return NCSCC_RC_FAILURE;
}
 
-   flags = fcntl(tcp_cb->DBSRsock, F_GETFD, 0);
-   if ((flags < 0) || (flags > 1)) {
-   syslog(LOG_ERR, "MDTM:TCP Unable to get the CLOEXEC Flag on 
DBSRsock  err :%s", strerror(errno));
-   close(tcp_cb->DBSRsock);
-   return NCSCC_RC_FAILURE;
-   } else {
-   if (fcntl(tcp_cb->DBSRsock, F_SETFD, (flags | FD_CLOEXEC)) == 
(-1)) {
-   syslog(LOG_ERR, "MDTM:TCP Unable to set the CLOEXEC 
Flag on DBSRsock err :%s", strerror(errno));
-   close(tcp_cb->DBSRsock);
-   return NCSCC_RC_FAILURE;
-   }
-   }
-
tcp_cb->adest = ((uint64_t)(nodeid)) << 32;
tcp_cb->adest |= mdtm_pid;
tcp_cb->node_id = nodeid;
diff --git a/osaf/libs/core/mds/mds_dt_tipc.c b/osaf/libs/core/mds/mds_dt_tipc.c
--- a/osaf/libs/core/mds/mds_dt_tipc.c
+++ b/osaf/libs/core/mds/mds_dt_tipc.c
@@ -45,6 +45,13 @@
 #include "mds_core.h"
 #include "osaf_utility.h"
 
+#ifndef SOCK_CLOEXEC
+enum {
+   SOCK_CLOEXEC = 0x8
+};
+#define SOCK_CLOEXEC SOCK_CLOEXEC
+#endif
+
 /*
 tipc_id will be 
 */
@@ -151,7 +158,6 @@ uint32_t mdtm_global_frag_num;
 uint32_t mdtm_tipc_init(NODE_ID nodeid, uint32_t *mds_tipc_ref)
 {
uint32_t tipc_node_id = 0;
-   int flags;
 
NCS_PATRICIA_PARAMS pat_tree_params;
 
@@ -176,48 +182,17 @@ uint32_t mdtm_tipc_init(NODE_ID nodeid, 
 
/* Create the sockets required for Binding, Send, receive and Discovery 
*/
 
-   tipc_cb.Dsock = socket(AF_TIPC, SOCK_SEQPACKET, 0);
+   tipc_cb.Dsock = socket(AF_TIPC, SOCK_SEQPACKET | SOCK_CLOEXEC, 0);
if (tipc_cb.Dsock < 0) {
syslog(LOG_ERR, "MDTM:TIPC Dsock Socket creation failed in 
MDTM_INIT err :%s", strerror(errno));
return NCSCC_RC_FAILURE;
}
-   tipc_cb.BSRsock = socket(AF_TIPC, SOCK_RDM, 0);
+   tipc_cb.BSRsock = socket(AF_TIPC, SOCK_RDM | SOCK_CLOEXEC, 0);
if (tipc_cb.BSRsock < 0) {
syslog(LOG_ERR, "MDTM:TIPC BSRsock Socket creation failed in 
MDTM_INIT err :%s", strerror(errno));
return NCSCC_RC_FAILURE;
}
 
-   flags = fcntl(tipc_cb.Dsock, F_GETFD, 0);
-   if ((flags < 0) || (flags > 1)) {
-   syslog(LOG_ERR, "MDTM:TIPC Unable to get the CLOEXEC Flag on 
Dsock err :%s", strerror(errno));
-   close(tipc_cb.Dsock);
-   close(tipc_cb.BSRsock);
-   return NCSCC_RC_FAILURE;
-   } else {
-   if (fcntl(tipc_cb.Dsock, F_SETFD, (flags | FD_CLOEXEC)) == 
(-1)) {
-   syslog(LOG_ERR, "MDTM:TIPC Unable to set the CLOEXEC 
Flag on Dsock err :%s", strerror(errno));
-   close(tipc_cb.Dsock);
-   close(tipc_cb.BSRsock);
-   return NCSCC_RC_FAILURE;
-   }
-   }
-
-   flags = fcntl(tipc_cb.BSRsock, F_GETFD, 0);
-   if ((flags < 0) || (flags > 1)) {
-   syslog(LOG_ERR, "MDTM:TIPC Unable to get the CLOEXEC Flag on 
BSRsock err :%s", strerror(errno));
-   close(tipc_cb.Dsock);
-   close(tipc_cb.BSRsock);
-   return NCSCC_RC_FAILURE;
-   } else {
-   if