[devel] [PATCH 1 of 1] log: fix logd crash on Active side [#2362]

[Canh Van Truong]

 src/log/logd/lgs_filehdl.cc |  12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)


The cause of issue is free cfg_namelist while struct dirent **cfg_namelist 
unallocated

diff --git a/src/log/logd/lgs_filehdl.cc b/src/log/logd/lgs_filehdl.cc
--- a/src/log/logd/lgs_filehdl.cc
+++ b/src/log/logd/lgs_filehdl.cc
@@ -797,14 +797,14 @@ int get_number_of_cfg_files_hdl(void *in
   } else {
 rc = (cfg_files - failed);
   }
-} 
-  }
+}
 
 done_cfg_free:
-  /* Free scandir allocated memory */
-  for (i = 0; i < cfg_files; i++)
-free(cfg_namelist[i]);
-  free(cfg_namelist);
+/* Free scandir allocated memory */
+for (i = 0; i < cfg_files; i++)
+  free(cfg_namelist[i]);
+free(cfg_namelist);
+  }
 
 done_log_free:
   /* Free scandir allocated memory */

--
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
___
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 log: fix logd crash on Active side [#2362]

[Canh Van Truong]

Summary: log: fix logd crash on Active side [#2362]
Review request for Trac Ticket(s): #2362
Peer Reviewer(s): Lennart, Vu, Mahesh
Pull request to: Vu
Affected branch(es): default
Development branch: default


Impacted area   Impact y/n

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


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

changeset 7e1a154d16851cdbfc3999eda94bf053d30d89f2
Author: Canh Van Truong 
Date:   Fri, 10 Mar 2017 20:37:52 +0700

log: fix logd crash on Active side [#2362]

The cause of issue is free cfg_namelist while struct dirent 
**cfg_namelist
unallocated


Complete diffstat:
--
 src/log/logd/lgs_filehdl.cc |  12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)


Testing Commands:
-
 <>


Testing, Expected Results:
--
 <>


Conditions of Submission:
-
Ack from 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.


--
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

[devel] [PATCH 1 of 1] amfd: honor PrefAssignedSU in N-Way and N-Way Active model during assignments [#2269]

[praveen . malviya]

 src/amf/amfd/sg.cc |  46 +-
 src/amf/amfd/sg.h  |   1 +
 src/amf/amfd/sg_nway_fsm.cc|  38 +++---
 src/amf/amfd/sg_nwayact_fsm.cc |  26 ++-
 4 files changed, 83 insertions(+), 28 deletions(-)


SG attribute saAmfSGNumPrefAssignedSUs is applicable to N-Way and N-Way Active 
model.
AMF is assigning more than saAmfSGNumPrefAssignedSUs in both N-Way and N-Way 
Active model.

Patch fixes this problem.

diff --git a/src/amf/amfd/sg.cc b/src/amf/amfd/sg.cc
--- a/src/amf/amfd/sg.cc
+++ b/src/amf/amfd/sg.cc
@@ -105,7 +105,7 @@ AVD_SG::AVD_SG():
saAmfSGAutoAdjust(SA_FALSE),
saAmfSGNumPrefActiveSUs(0),
saAmfSGNumPrefStandbySUs(0),
-   saAmfSGNumPrefInserviceSUs(~0),
+   saAmfSGNumPrefInserviceSUs(0),
saAmfSGNumPrefAssignedSUs(0),
saAmfSGMaxActiveSIsperSU(0),
saAmfSGMaxStandbySIsperSU(0),
@@ -941,16 +941,16 @@ static void ccb_apply_modify_hdlr(CcbUti
TRACE("Modified saAmfSGNumPrefStandbySUs is 
'%u'", sg->saAmfSGNumPrefStandbySUs);
} else if (!strcmp(attribute->attrName, 
"saAmfSGNumPrefInserviceSUs")) {
if (value_is_deleted)
-   sg->saAmfSGNumPrefInserviceSUs = ~0;
+   sg->saAmfSGNumPrefInserviceSUs = 0; 
//default value for internal use.
else
sg->saAmfSGNumPrefInserviceSUs = 
*((SaUint32T *)value);
-   TRACE("Modified saAmfSGNumPrefInserviceSUs is 
'%u'", sg->saAmfSGNumPrefInserviceSUs);
+   TRACE("Modified saAmfSGNumPrefInserviceSUs is 
'%u'", sg->pref_inservice_sus());
} else if (!strcmp(attribute->attrName, 
"saAmfSGNumPrefAssignedSUs")) {
if (value_is_deleted)
-   sg->saAmfSGNumPrefAssignedSUs = 
sg->saAmfSGNumPrefInserviceSUs;
+   sg->saAmfSGNumPrefAssignedSUs = 0; 
//default value for internal use.
else
sg->saAmfSGNumPrefAssignedSUs = 
*((SaUint32T *)value);
-   TRACE("Modified saAmfSGNumPrefAssignedSUs is 
'%u'", sg->saAmfSGNumPrefAssignedSUs);
+   TRACE("Modified saAmfSGNumPrefAssignedSUs is 
'%u'", sg->pref_assigned_sus());
} else if (!strcmp(attribute->attrName, 
"saAmfSGMaxActiveSIsperSU")) {
if (value_is_deleted)
sg->saAmfSGMaxActiveSIsperSU = -1;
@@ -1043,10 +1043,10 @@ static void ccb_apply_modify_hdlr(CcbUti
 
if (!strcmp(attribute->attrName, 
"saAmfSGNumPrefInserviceSUs")) {
if (value_is_deleted)
-   sg->saAmfSGNumPrefInserviceSUs = ~0;
+   sg->saAmfSGNumPrefInserviceSUs = 0;
else
sg->saAmfSGNumPrefInserviceSUs = 
*((SaUint32T *)value);
-   TRACE("Modified saAmfSGNumPrefInserviceSUs is 
'%u'", sg->saAmfSGNumPrefInserviceSUs);
+   TRACE("Modified saAmfSGNumPrefInserviceSUs is 
'%u'", sg->pref_inservice_sus());
 
if (avd_cb->avail_state_avd == 
SA_AMF_HA_ACTIVE)  {
if (avd_sg_app_su_inst_func(avd_cb, sg) 
!= NCSCC_RC_SUCCESS) {
@@ -1209,7 +1209,7 @@ static void sg_app_sg_admin_unlock_inst(
 
if (su->saAmfSUPreInstantiable == true) {
if (su->su_on_node->node_state == 
AVD_AVND_STATE_PRESENT) {
-   if 
(su->sg_of_su->saAmfSGNumPrefInserviceSUs > su_try_inst) {
+   if (su->sg_of_su->pref_inservice_sus() 
> su_try_inst) {
if (avd_snd_presence_msg(cb, 
su, false) != NCSCC_RC_SUCCESS) {
LOG_NO("%s: Failed to 
send Instantiation order of '%s' to %x",

__FUNCTION__, su->name.c_str(),
@@ -1866,17 +1866,6 @@ void avd_sg_adjust_config(AVD_SG *sg)
}
}
}
-
-   /* adjust saAmfSGNumPrefAssignedSUs if not configured, only applicable 
for
-* the N-way and N-way active redundancy models
-*/
-   if ((sg->saAmfSGNumPrefAssignedSUs == 0) &&
-   ((sg->sg_type->saAmfSgtRedundancyModel == 
SA_AMF_N_WAY_REDUNDANCY_MODEL) ||
-   

[devel] [PATCH 0 of 1] Review Request for amfd: honor PrefAssignedSU in N-Way and N-Way Active model during assignments [#2269].

[praveen . malviya]

Summary: amfd: honor PrefAssignedSU in N-Way and N-Way Active model during 
assignments [#2269]. 
Review request for Trac Ticket(s): #2269 
Peer Reviewer(s): AMF devs 
Pull request to: <>
Affected branch(es): ALL 
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 62b35316b2e40dff6098f4385e2073f2f1e5a11b
Author: Praveen Malviya 
Date:   Fri, 10 Mar 2017 16:09:20 +0530

amfd: honor PrefAssignedSU in N-Way and N-Way Active model during
assignments [#2269].

SG attribute saAmfSGNumPrefAssignedSUs is applicable to N-Way and N-Way
Active model. AMF is assigning more than saAmfSGNumPrefAssignedSUs in 
both
N-Way and N-Way Active model.

Patch fixes this problem.


Complete diffstat:
--
 src/amf/amfd/sg.cc |  46 
+++---
 src/amf/amfd/sg.h  |   1 +
 src/amf/amfd/sg_nway_fsm.cc|  38 ++
 src/amf/amfd/sg_nwayact_fsm.cc |  26 +-
 4 files changed, 83 insertions(+), 28 deletions(-)


Testing Commands:
-
Brought up N-Way and N-Way active models:
1)with siranked su configured.
2)with equal distribution enabled.


Testing, Expected Results:
--
PASS.
AMF assigns only PrefAssignedSus.

Conditions of Submission:
-
Ack from reviewers.

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.


--
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
___
Opensaf-devel mailing list

Re: [devel] [PATCH 0 of 1] Review Request for smfd: fix campaign suspend when last step of procedure is executing [#2332]

[Neelakanta Reddy]

Hi Alex,

Reviewed and tested the patch.
Ack.

/Neel.

On 2017/03/08 02:51 AM, Alex Jones wrote:
> Summary: smfd: fix campaign suspend when last step of procedure is executing
> Review request for Trac Ticket(s): 2332
> Peer Reviewer(s): Neel, Lennart, Rafael
> Pull request to:
> Affected branch(es): default, 5.1, 5.0
> 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):
> -
> There is a comment being removed which explains why this shouldn't be done,
> but I don't understand why.
>
> changeset 7bc8dd36c9b2760a15bb0cd08a57b8fff8a26e87
> Author:   Alex Jones 
> Date: Tue, 07 Mar 2017 16:05:07 -0500
>
>   smfd: fix campaign suspend when last step of procedure is executing 
> [#2332]
>
>   Campaign is in suspended state (after ADMIN_SUSPEND sent to it), but no
>   procedure is in suspended state.
>
>   The code in SmfProcStateExecuting::executeStep and
>   SmfProcStateRollingBack::rollbackStep does not properly handle the last
>   step. If the last step completes the EXECUTE message is not sent back 
> to the
>   procedure thread to see if a pending suspend is sitting there, but 
> continues
>   on with the wrapup and returns COMPLETED. Then the waiting SUSPEND 
> message
>   is executed, but the procedure is in COMPLETED state, so it is ignored.
>
>   Solution is to remove the code which checks if this is the last step.
>
>
> Complete diffstat:
> --
>   src/smf/smfd/SmfProcState.cc |  48 
> ++--
>   1 files changed, 14 insertions(+), 34 deletions(-)
>
>
> Testing Commands:
> -
> (1) run the campaign_rolling_nodes_os_installremove.xml campaign in the 
> samples directory
> (2) after the second node has rebooted, do "smf-adm suspend "
> (3) verify that the campaign has successfully suspended
>
>
> Testing, Expected Results:
> --
> (1) "smf-state proc all" will show no procedure in suspended state
> (2) "smf-adm execute " will fail
>
>
> 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 

Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

[Nagendra Kumar]

Hi Gary,
Sure. I will change it.

Thanks
-Nagu

- Original Message -
From: gary@dektech.com.au
To: nagendr...@oracle.com
Cc: minh.c...@dektech.com.au, opensaf-devel@lists.sourceforge.net, 
praveen.malv...@oracle.com, hans.nordeb...@ericsson.com
Sent: Friday, March 10, 2017 2:50:12 PM GMT +05:30 Chennai, Kolkata, Mumbai, 
New Delhi
Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

Maybe something like:

diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc
--- a/src/amf/amfd/role.cc
+++ b/src/amf/amfd/role.cc
@@ -130,8 +130,12 @@ void avd_role_change_evh(AVD_CL_CB *cb, 
(role == SA_AMF_HA_ACTIVE) && (cb->avail_state_avd == 
SA_AMF_HA_STANDBY)) {
if (true == cb->swap_switch ) {
/* swap resulted Switch  standby -> Active */
-   amfd_switch_stdby_actv(cb);
-   status = NCSCC_RC_SUCCESS;
+   LOG_NO("Switching StandBy --> Active State");   
+   status = amfd_switch_stdby_actv(cb);
+   if (status == NCSCC_RC_SUCCESS) {
+   LOG_NO("Controller switch over done");
+   saflog(LOG_NOTICE, amfSvcUsrName, 
"Controller switch over done at %x", cb->node_id_avd);
+   }
goto done;
}
}
@@ -1162,8 +1166,6 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C

TRACE_ENTER();
 
-   LOG_NO("Switching StandBy --> Active State");
-
/*
 * Check whether Standby is in sync with Active. If yes then
 * proceed further. Else return failure.
@@ -1282,9 +1284,6 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C
}
}
 
-   LOG_NO("Controller switch over done");
-   saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch over done at %x", 
cb->node_id_avd);
-
TRACE_LEAVE();
return NCSCC_RC_SUCCESS;


Thanks
Gary


-Original Message-
From: Nagendra Kumar 
Date: Friday, 10 March 2017 at 8:11 pm
To: gary 
Cc: , , 
, 
Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

Hi Gary,
 I got it, thanks. I can move the below line before calling the 
function:

LOG_NO("Switching StandBy --> Active State");

But, I think the below line is difficult to moved out as it is dependent of 
some preceding functions success:
 +  LOG_NO("Controller switch over done");
 +  saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch
 over done at %x", cb->node_id_avd);

Thanks
-Nagu


- Original Message -
From: gary@dektech.com.au
To: nagendr...@oracle.com, hans.nordeb...@ericsson.com, 
praveen.malv...@oracle.com, minh.c...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Sent: Friday, March 10, 2017 2:29:59 PM GMT +05:30 Chennai, Kolkata, 
Mumbai, New Delhi
Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set 
[#2338]

Hi Nagu

Sorry I wasn’t very clear. I meant moving the 3 or so lines of LOG_NO from 
amfd_switch_stdby_actv() to where you call amfd_switch_stdby_actv(…, true).

Thanks
Gary

-Original Message-
From: Nagendra Kumar  on behalf of Nagendra Kumar 

Date: Friday, 10 March 2017 at 7:50 pm
To: gary , , Praveen 
Malviya , 
Cc: 
Subject: RE: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set 
[#2338]

Hi Gary,
Thank you for that.
I also thought of the same thing, but since it is old log and provides 
a good information to the user about switchover, so I couldn't remove it.

Thanks
-Nagu

> -Original Message-
> From: Gary Lee [mailto:gary@dektech.com.au]
> Sent: 10 March 2017 06:26
> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for 
avd_imm_applier_set
> [#2338]
> 
> HI Nagu
> 
> Ack (reviewed + legacy tests run).
> 
> Minor comment: perhaps it’s easier to take the log message out of
> amfd_switch_stdby_actv() than add a parameter, since it’s only called 
from 2
> places.
> 
> Thanks
> Gary
> 
> -Original Message-
> From: 

Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

[Gary Lee]

Maybe something like:

diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc
--- a/src/amf/amfd/role.cc
+++ b/src/amf/amfd/role.cc
@@ -130,8 +130,12 @@ void avd_role_change_evh(AVD_CL_CB *cb, 
(role == SA_AMF_HA_ACTIVE) && (cb->avail_state_avd == 
SA_AMF_HA_STANDBY)) {
if (true == cb->swap_switch ) {
/* swap resulted Switch  standby -> Active */
-   amfd_switch_stdby_actv(cb);
-   status = NCSCC_RC_SUCCESS;
+   LOG_NO("Switching StandBy --> Active State");   
+   status = amfd_switch_stdby_actv(cb);
+   if (status == NCSCC_RC_SUCCESS) {
+   LOG_NO("Controller switch over done");
+   saflog(LOG_NOTICE, amfSvcUsrName, 
"Controller switch over done at %x", cb->node_id_avd);
+   }
goto done;
}
}
@@ -1162,8 +1166,6 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C

TRACE_ENTER();
 
-   LOG_NO("Switching StandBy --> Active State");
-
/*
 * Check whether Standby is in sync with Active. If yes then
 * proceed further. Else return failure.
@@ -1282,9 +1284,6 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C
}
}
 
-   LOG_NO("Controller switch over done");
-   saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch over done at %x", 
cb->node_id_avd);
-
TRACE_LEAVE();
return NCSCC_RC_SUCCESS;


Thanks
Gary


-Original Message-
From: Nagendra Kumar 
Date: Friday, 10 March 2017 at 8:11 pm
To: gary 
Cc: , , 
, 
Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

Hi Gary,
 I got it, thanks. I can move the below line before calling the 
function:

LOG_NO("Switching StandBy --> Active State");

But, I think the below line is difficult to moved out as it is dependent of 
some preceding functions success:
 +  LOG_NO("Controller switch over done");
 +  saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch
 over done at %x", cb->node_id_avd);

Thanks
-Nagu


- Original Message -
From: gary@dektech.com.au
To: nagendr...@oracle.com, hans.nordeb...@ericsson.com, 
praveen.malv...@oracle.com, minh.c...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Sent: Friday, March 10, 2017 2:29:59 PM GMT +05:30 Chennai, Kolkata, 
Mumbai, New Delhi
Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set 
[#2338]

Hi Nagu

Sorry I wasn’t very clear. I meant moving the 3 or so lines of LOG_NO from 
amfd_switch_stdby_actv() to where you call amfd_switch_stdby_actv(…, true).

Thanks
Gary

-Original Message-
From: Nagendra Kumar  on behalf of Nagendra Kumar 

Date: Friday, 10 March 2017 at 7:50 pm
To: gary , , Praveen 
Malviya , 
Cc: 
Subject: RE: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set 
[#2338]

Hi Gary,
Thank you for that.
I also thought of the same thing, but since it is old log and provides 
a good information to the user about switchover, so I couldn't remove it.

Thanks
-Nagu

> -Original Message-
> From: Gary Lee [mailto:gary@dektech.com.au]
> Sent: 10 March 2017 06:26
> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for 
avd_imm_applier_set
> [#2338]
> 
> HI Nagu
> 
> Ack (reviewed + legacy tests run).
> 
> Minor comment: perhaps it’s easier to take the log message out of
> amfd_switch_stdby_actv() than add a parameter, since it’s only called 
from 2
> places.
> 
> Thanks
> Gary
> 
> -Original Message-
> From: 
> Date: Wednesday, 8 March 2017 at 6:49 pm
> To: , ,
> , gary 
> Cc: 
> Subject: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
>  src/amf/amfd/role.cc |  22 --
>  

Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

[Nagendra Kumar]

Hi Gary,
 I got it, thanks. I can move the below line before calling the 
function:

LOG_NO("Switching StandBy --> Active State");

But, I think the below line is difficult to moved out as it is dependent of 
some preceding functions success:
 +  LOG_NO("Controller switch over done");
 +  saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch
 over done at %x", cb->node_id_avd);

Thanks
-Nagu


- Original Message -
From: gary@dektech.com.au
To: nagendr...@oracle.com, hans.nordeb...@ericsson.com, 
praveen.malv...@oracle.com, minh.c...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Sent: Friday, March 10, 2017 2:29:59 PM GMT +05:30 Chennai, Kolkata, Mumbai, 
New Delhi
Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

Hi Nagu

Sorry I wasn’t very clear. I meant moving the 3 or so lines of LOG_NO from 
amfd_switch_stdby_actv() to where you call amfd_switch_stdby_actv(…, true).

Thanks
Gary

-Original Message-
From: Nagendra Kumar  on behalf of Nagendra Kumar 

Date: Friday, 10 March 2017 at 7:50 pm
To: gary , , Praveen 
Malviya , 
Cc: 
Subject: RE: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

Hi Gary,
Thank you for that.
I also thought of the same thing, but since it is old log and provides a 
good information to the user about switchover, so I couldn't remove it.

Thanks
-Nagu

> -Original Message-
> From: Gary Lee [mailto:gary@dektech.com.au]
> Sent: 10 March 2017 06:26
> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
> HI Nagu
> 
> Ack (reviewed + legacy tests run).
> 
> Minor comment: perhaps it’s easier to take the log message out of
> amfd_switch_stdby_actv() than add a parameter, since it’s only called 
from 2
> places.
> 
> Thanks
> Gary
> 
> -Original Message-
> From: 
> Date: Wednesday, 8 March 2017 at 6:49 pm
> To: , ,
> , gary 
> Cc: 
> Subject: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
>  src/amf/amfd/role.cc |  22 --
>  src/amf/amfd/role.h  |   2 +-
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc
> --- a/src/amf/amfd/role.cc
> +++ b/src/amf/amfd/role.cc
> @@ -130,7 +130,7 @@ void avd_role_change_evh(AVD_CL_CB *cb,
>   (role == SA_AMF_HA_ACTIVE) && (cb->avail_state_avd ==
> SA_AMF_HA_STANDBY)) {
>   if (true == cb->swap_switch ) {
>   /* swap resulted Switch  standby -> Active */
> - amfd_switch_stdby_actv(cb);
> + amfd_switch_stdby_actv(cb, true);
>   status = NCSCC_RC_SUCCESS;
>   goto done;
>   }
> @@ -803,6 +803,12 @@ try_again:
>  failed and amf reinitializes imm interface 
and
>  set applier in avd_imm_reinit_bg_thread. Imm 
may
>  return ERR_EXIST or INVALID_PARAM. */
> + TRACE("ERR_EXIST or INVALID_PARAM");
> + } else if (rc == SA_AIS_ERR_TIMEOUT) {
> + /* Let it proceed as there may be a case of 
Immd not
> +reachable i.e. node might have gone down 
during
> +switchover. */
> + TRACE("SA_AIS_ERR_TIMEOUT");
>   } else
>   osafassert(0);
>   } else
> @@ -1147,6 +1153,7 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
>   *   change from standby to active in liue of SI SWAP Action
>   *
>   * Input: cb - AVD control block pointer.
> + * Input: switch_flag - Whether switchover passed or failed.
>   *
>   * Returns: NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE.
>   *
> @@ -1155,14 +1162,15 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
>   *
> 
> **
> /
> 
> -uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb)
  

Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

[Gary Lee]

Hi Nagu

Sorry I wasn’t very clear. I meant moving the 3 or so lines of LOG_NO from 
amfd_switch_stdby_actv() to where you call amfd_switch_stdby_actv(…, true).

Thanks
Gary

-Original Message-
From: Nagendra Kumar  on behalf of Nagendra Kumar 

Date: Friday, 10 March 2017 at 7:50 pm
To: gary , , Praveen 
Malviya , 
Cc: 
Subject: RE: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

Hi Gary,
Thank you for that.
I also thought of the same thing, but since it is old log and provides a 
good information to the user about switchover, so I couldn't remove it.

Thanks
-Nagu

> -Original Message-
> From: Gary Lee [mailto:gary@dektech.com.au]
> Sent: 10 March 2017 06:26
> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
> HI Nagu
> 
> Ack (reviewed + legacy tests run).
> 
> Minor comment: perhaps it’s easier to take the log message out of
> amfd_switch_stdby_actv() than add a parameter, since it’s only called 
from 2
> places.
> 
> Thanks
> Gary
> 
> -Original Message-
> From: 
> Date: Wednesday, 8 March 2017 at 6:49 pm
> To: , ,
> , gary 
> Cc: 
> Subject: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
>  src/amf/amfd/role.cc |  22 --
>  src/amf/amfd/role.h  |   2 +-
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc
> --- a/src/amf/amfd/role.cc
> +++ b/src/amf/amfd/role.cc
> @@ -130,7 +130,7 @@ void avd_role_change_evh(AVD_CL_CB *cb,
>   (role == SA_AMF_HA_ACTIVE) && (cb->avail_state_avd ==
> SA_AMF_HA_STANDBY)) {
>   if (true == cb->swap_switch ) {
>   /* swap resulted Switch  standby -> Active */
> - amfd_switch_stdby_actv(cb);
> + amfd_switch_stdby_actv(cb, true);
>   status = NCSCC_RC_SUCCESS;
>   goto done;
>   }
> @@ -803,6 +803,12 @@ try_again:
>  failed and amf reinitializes imm interface 
and
>  set applier in avd_imm_reinit_bg_thread. Imm 
may
>  return ERR_EXIST or INVALID_PARAM. */
> + TRACE("ERR_EXIST or INVALID_PARAM");
> + } else if (rc == SA_AIS_ERR_TIMEOUT) {
> + /* Let it proceed as there may be a case of 
Immd not
> +reachable i.e. node might have gone down 
during
> +switchover. */
> + TRACE("SA_AIS_ERR_TIMEOUT");
>   } else
>   osafassert(0);
>   } else
> @@ -1147,6 +1153,7 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
>   *   change from standby to active in liue of SI SWAP Action
>   *
>   * Input: cb - AVD control block pointer.
> + * Input: switch_flag - Whether switchover passed or failed.
>   *
>   * Returns: NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE.
>   *
> @@ -1155,14 +1162,15 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
>   *
> 
> **
> /
> 
> -uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb)
> +uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb, bool switchover_flag)
>  {
>   uint32_t status = NCSCC_RC_SUCCESS;
>   SaAisErrorT rc;
> 
>   TRACE_ENTER();
> 
> - LOG_NO("Switching StandBy --> Active State");
> + if (switchover_flag)
> + LOG_NO("Switching StandBy --> Active State");
> 
>   /*
>* Check whether Standby is in sync with Active. If yes then
> @@ -1282,8 +1290,10 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C
>   }
>   }
> 
> - LOG_NO("Controller switch over done");
> - saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch over done
> at %x", cb->node_id_avd);
> + if (switchover_flag) {

Re: [devel] [PATCH 0 of 3] Review Request for amf: Fix all Cppcheck 1.77 issues [#2341] V1

[Gary Lee]

Hi Mahesh

I’m still going through it. Seems to be mainly asserts and coredumps.

Eg. csi->num_attributes is not set properly. The fix below seems to work.

diff --git a/src/amf/amfd/csi.cc b/src/amf/amfd/csi.cc
--- a/src/amf/amfd/csi.cc
+++ b/src/amf/amfd/csi.cc
@@ -1399,12 +1399,13 @@ void avd_csi_remove_csiattr(AVD_CSI *csi
 
 void avd_csi_add_csiattr(AVD_CSI *csi, AVD_CSI_ATTR *csiattr)
 {
-   int cnt = 0;
+   int cnt = 1;
AVD_CSI_ATTR *ptr;
 
TRACE_ENTER();
/* Count number of attributes (multivalue) */
ptr = csiattr;
+   osafassert(ptr != nullptr);
while (ptr->attr_next != nullptr) {
cnt++;
ptr = ptr->attr_next;

Coredumps:

==428== Thread 1:
==428== Invalid free() / delete / delete[] / realloc()
==428==at 0x4C2C2BC: operator delete(void*) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==428==by 0x132CD1: avnd_comp_pm_rec_del(avnd_cb_tag*, avnd_comp_tag*, 
avnd_pm_rec*) (cpm.cc:148)
==428==by 0x1434C4: avnd_evt_pid_exit_evh(avnd_cb_tag*, avnd_evt_tag*) 
(mon.cc:408)
==428==by 0x14061E: avnd_evt_process (main.cc:665)
==428==by 0x14061E: avnd_main_process() (main.cc:616)
==428==by 0x1164FE: main (main.cc:206)
==428==  Address 0x81c7c70 is 0 bytes inside a block of size 72 free'd
==428==at 0x4C2C2BC: operator delete(void*) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==428==by 0x132B68: avnd_pm_rec_free(ncs_db_link_list_node*) (cpm.cc:86)
==428==by 0x56B9B9A: ncs_db_link_list_del (ncsdlib.c:144)
==428==by 0x132C9E: avnd_comp_pm_rec_del(avnd_cb_tag*, avnd_comp_tag*, 
avnd_pm_rec*) (cpm.cc:143)
==428==by 0x1434C4: avnd_evt_pid_exit_evh(avnd_cb_tag*, avnd_evt_tag*) 
(mon.cc:408)
==428==by 0x14061E: avnd_evt_process (main.cc:665)
==428==by 0x14061E: avnd_main_process() (main.cc:616)
==428==by 0x1164FE: main (main.cc:206)
==428==
==428== Invalid free() / delete / delete[] / realloc()
==428==at 0x4C2C2BC: operator delete(void*) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==428==by 0x132CD1: avnd_comp_pm_rec_del(avnd_cb_tag*, avnd_comp_tag*, 
avnd_pm_rec*) (cpm.cc:148)
==428==by 0x132F3B: avnd_comp_pm_stop_process(avnd_cb_tag*, avnd_comp_tag*, 
avsv_amf_pm_stop_param_tag*, SaAisErrorT*) (cpm.cc:227)
==428==by 0x133994: avnd_evt_ava_pm_stop_evh(avnd_cb_tag*, avnd_evt_tag*) 
(cpm.cc:485)
==428==by 0x14061E: avnd_evt_process (main.cc:665)
==428==by 0x14061E: avnd_main_process() (main.cc:616)
==428==by 0x1164FE: main (main.cc:206)
==428==  Address 0x81ccea0 is 0 bytes inside a block of size 72 free'd
==428==at 0x4C2C2BC: operator delete(void*) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==428==by 0x132B68: avnd_pm_rec_free(ncs_db_link_list_node*) (cpm.cc:86)
==428==by 0x56B9B9A: ncs_db_link_list_del (ncsdlib.c:144)
==428==by 0x132C9E: avnd_comp_pm_rec_del(avnd_cb_tag*, avnd_comp_tag*, 
avnd_pm_rec*) (cpm.cc:143)
==428==by 0x132F3B: avnd_comp_pm_stop_process(avnd_cb_tag*, avnd_comp_tag*, 
avsv_amf_pm_stop_param_tag*, SaAisErrorT*) (cpm.cc:227)
==428==by 0x133994: avnd_evt_ava_pm_stop_evh(avnd_cb_tag*, avnd_evt_tag*) 
(cpm.cc:485)
==428==by 0x14061E: avnd_evt_process (main.cc:665)
==428==by 0x14061E: avnd_main_process() (main.cc:616)
==428==by 0x1164FE: main (main.cc:206)

-Original Message-
From: A V Mahesh 
Organization: Oracle Corporation
Date: Friday, 10 March 2017 at 6:42 pm
To: gary , , 
, 
Cc: 
Subject: Re: [PATCH 0 of 3] Review Request for amf: Fix all Cppcheck 1.77 
issues [#2341] V1

Hi Gary,

Thanks for the initial inputs.

The patch doesn't contain any in-service /backward compatible change , 
just wondering
how this patch broken the  legacy Application , can you please 
code-snippet of your legacy Application,
that breaking the patch.

-AVM

On 3/10/2017 10:59 AM, Gary Lee wrote:
> Hi Mahesh
>
> Some of our legacy tests are failing after applying this series.
>
> I will do some debugging and send back details.
>
> Thanks
> Gary
>
> -Original Message-
> From: A V Mahesh 
> Date: Wednesday, 8 March 2017 at 11:28 pm
> To: , gary , 
, 
> Cc: 
> Subject: [PATCH 0 of 3] Review Request for amf: Fix all Cppcheck 1.77 
issues [#2341] V1
>
>  Summary:amf: Fix all Cppcheck 1.77 issues [#2341] V1
>  Review request for Trac Ticket(s): #2341
>  Peer Reviewer(s): Amf Dev
>  Pull request to: <>
>  Affected branch(es): default, 5.2
>  Development branch: default
>

Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

[Nagendra Kumar]

Also since, there were confusing logs as :
1. ROLE SWITCH Quiesced --> Active
2. Switching StandBy --> Active State

So, I thought, I should comment #2 as it is not giving the right information as 
Amfd is in Quisced and not in Standby state.

Thanks
-Nagu

> -Original Message-
> From: Nagendra Kumar
> Sent: 10 March 2017 14:21
> To: Gary Lee; hans.nordeb...@ericsson.com; Praveen Malviya;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for
> avd_imm_applier_set [#2338]
> 
> Hi Gary,
>   Thank you for that.
> I also thought of the same thing, but since it is old log and provides a good
> information to the user about switchover, so I couldn't remove it.
> 
> Thanks
> -Nagu
> 
> > -Original Message-
> > From: Gary Lee [mailto:gary@dektech.com.au]
> > Sent: 10 March 2017 06:26
> > To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> > minh.c...@dektech.com.au
> > Cc: opensaf-devel@lists.sourceforge.net
> > Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for
> > avd_imm_applier_set [#2338]
> >
> > HI Nagu
> >
> > Ack (reviewed + legacy tests run).
> >
> > Minor comment: perhaps it’s easier to take the log message out of
> > amfd_switch_stdby_actv() than add a parameter, since it’s only called
> > from 2 places.
> >
> > Thanks
> > Gary
> >
> > -Original Message-
> > From: 
> > Date: Wednesday, 8 March 2017 at 6:49 pm
> > To: , ,
> > , gary 
> > Cc: 
> > Subject: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> > [#2338]
> >
> >  src/amf/amfd/role.cc |  22 --
> >  src/amf/amfd/role.h  |   2 +-
> >  2 files changed, 17 insertions(+), 7 deletions(-)
> >
> >
> > diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc
> > --- a/src/amf/amfd/role.cc
> > +++ b/src/amf/amfd/role.cc
> > @@ -130,7 +130,7 @@ void avd_role_change_evh(AVD_CL_CB *cb,
> > (role == SA_AMF_HA_ACTIVE) && (cb->avail_state_avd ==
> > SA_AMF_HA_STANDBY)) {
> > if (true == cb->swap_switch ) {
> > /* swap resulted Switch  standby -> Active */
> > -   amfd_switch_stdby_actv(cb);
> > +   amfd_switch_stdby_actv(cb, true);
> > status = NCSCC_RC_SUCCESS;
> > goto done;
> > }
> > @@ -803,6 +803,12 @@ try_again:
> >failed and amf reinitializes imm interface 
> > and
> >set applier in avd_imm_reinit_bg_thread. Imm 
> > may
> >return ERR_EXIST or INVALID_PARAM. */
> > +   TRACE("ERR_EXIST or INVALID_PARAM");
> > +   } else if (rc == SA_AIS_ERR_TIMEOUT) {
> > +   /* Let it proceed as there may be a case of 
> > Immd not
> > +  reachable i.e. node might have gone down 
> > during
> > +  switchover. */
> > +   TRACE("SA_AIS_ERR_TIMEOUT");
> > } else
> > osafassert(0);
> > } else
> > @@ -1147,6 +1153,7 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
> >   *   change from standby to active in liue of SI SWAP Action
> >   *
> >   * Input: cb - AVD control block pointer.
> > + * Input: switch_flag - Whether switchover passed or failed.
> >   *
> >   * Returns: NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE.
> >   *
> > @@ -1155,14 +1162,15 @@ uint32_t
> amfd_switch_qsd_stdby(AVD_CL_CB
> >   *
> >
> >
> **
> > /
> >
> > -uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb)
> > +uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb, bool
> switchover_flag)
> >  {
> > uint32_t status = NCSCC_RC_SUCCESS;
> > SaAisErrorT rc;
> >
> > TRACE_ENTER();
> >
> > -   LOG_NO("Switching StandBy --> Active State");
> > +   if (switchover_flag)
> > +   LOG_NO("Switching StandBy --> Active State");
> >
> > /*
> >  * Check whether Standby is in sync with Active. If yes then
> > @@ -1282,8 +1290,10 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C
> > }
> > }
> >
> > -   LOG_NO("Controller switch over done");
> > -   saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch over done
> > at %x", cb->node_id_avd);
> > +   if (switchover_flag) {
> > +   LOG_NO("Controller switch over done");
> > +   saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch
> > over done at %x", 

Re: [devel] [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set [#2338]

[Nagendra Kumar]

Hi Gary,
Thank you for that.
I also thought of the same thing, but since it is old log and provides a good 
information to the user about switchover, so I couldn't remove it.

Thanks
-Nagu

> -Original Message-
> From: Gary Lee [mailto:gary@dektech.com.au]
> Sent: 10 March 2017 06:26
> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
> HI Nagu
> 
> Ack (reviewed + legacy tests run).
> 
> Minor comment: perhaps it’s easier to take the log message out of
> amfd_switch_stdby_actv() than add a parameter, since it’s only called from 2
> places.
> 
> Thanks
> Gary
> 
> -Original Message-
> From: 
> Date: Wednesday, 8 March 2017 at 6:49 pm
> To: , ,
> , gary 
> Cc: 
> Subject: [PATCH 1 of 1] amfd: handle TIMEOUT for avd_imm_applier_set
> [#2338]
> 
>  src/amf/amfd/role.cc |  22 --
>  src/amf/amfd/role.h  |   2 +-
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc
> --- a/src/amf/amfd/role.cc
> +++ b/src/amf/amfd/role.cc
> @@ -130,7 +130,7 @@ void avd_role_change_evh(AVD_CL_CB *cb,
>   (role == SA_AMF_HA_ACTIVE) && (cb->avail_state_avd ==
> SA_AMF_HA_STANDBY)) {
>   if (true == cb->swap_switch ) {
>   /* swap resulted Switch  standby -> Active */
> - amfd_switch_stdby_actv(cb);
> + amfd_switch_stdby_actv(cb, true);
>   status = NCSCC_RC_SUCCESS;
>   goto done;
>   }
> @@ -803,6 +803,12 @@ try_again:
>  failed and amf reinitializes imm interface and
>  set applier in avd_imm_reinit_bg_thread. Imm may
>  return ERR_EXIST or INVALID_PARAM. */
> + TRACE("ERR_EXIST or INVALID_PARAM");
> + } else if (rc == SA_AIS_ERR_TIMEOUT) {
> + /* Let it proceed as there may be a case of Immd not
> +reachable i.e. node might have gone down during
> +switchover. */
> + TRACE("SA_AIS_ERR_TIMEOUT");
>   } else
>   osafassert(0);
>   } else
> @@ -1147,6 +1153,7 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
>   *   change from standby to active in liue of SI SWAP Action
>   *
>   * Input: cb - AVD control block pointer.
> + * Input: switch_flag - Whether switchover passed or failed.
>   *
>   * Returns: NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE.
>   *
> @@ -1155,14 +1162,15 @@ uint32_t amfd_switch_qsd_stdby(AVD_CL_CB
>   *
> 
> **
> /
> 
> -uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb)
> +uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb, bool switchover_flag)
>  {
>   uint32_t status = NCSCC_RC_SUCCESS;
>   SaAisErrorT rc;
> 
>   TRACE_ENTER();
> 
> - LOG_NO("Switching StandBy --> Active State");
> + if (switchover_flag)
> + LOG_NO("Switching StandBy --> Active State");
> 
>   /*
>* Check whether Standby is in sync with Active. If yes then
> @@ -1282,8 +1290,10 @@ uint32_t amfd_switch_stdby_actv(AVD_CL_C
>   }
>   }
> 
> - LOG_NO("Controller switch over done");
> - saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch over done
> at %x", cb->node_id_avd);
> + if (switchover_flag) {
> + LOG_NO("Controller switch over done");
> + saflog(LOG_NOTICE, amfSvcUsrName, "Controller switch
> over done at %x", cb->node_id_avd);
> + }
> 
>   TRACE_LEAVE();
>   return NCSCC_RC_SUCCESS;
> @@ -1311,7 +1321,7 @@ uint32_t amfd_switch_qsd_actv (AVD_CL_CB
>   if (NCSCC_RC_SUCCESS != avd_rde_set_role(SA_AMF_HA_ACTIVE)) {
>   LOG_ER("rde role change failed from qsd -> actv");
>   }
> - return amfd_switch_stdby_actv(cb);
> + return amfd_switch_stdby_actv(cb, false);
>  }
> 
> 
> diff --git a/src/amf/amfd/role.h b/src/amf/amfd/role.h
> --- a/src/amf/amfd/role.h
> +++ b/src/amf/amfd/role.h
> @@ -31,7 +31,7 @@ extern uint32_t avd_d2d_chg_role_rsp(AVD
>  extern uint32_t avd_d2d_chg_role_req(AVD_CL_CB *cb,
> AVD_ROLE_CHG_CAUSE_T cause, SaAmfHAStateT role);
> 
>  extern uint32_t amfd_switch_qsd_stdby(AVD_CL_CB *cb);
> -extern uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb);
> +extern uint32_t amfd_switch_stdby_actv(AVD_CL_CB *cb, bool);
>  extern uint32_t amfd_switch_qsd_actv(AVD_CL_CB *cb);
>  extern uint32_t 

Re: [devel] [PATCH 0 of 3] Review Request for amf: Fix all Cppcheck 1.77 issues [#2341] V1

[A V Mahesh]

Hi Gary,

One of the possibility of patch  breaking the legacy Application is 
following change,
try to rollback this change and see:

===

diff --git a/src/amf/agent/ava_op.cc b/src/amf/agent/ava_op.cc
--- a/src/amf/agent/ava_op.cc
+++ b/src/amf/agent/ava_op.cc
@@ -117,7 +117,6 @@ uint32_t ava_avnd_msg_prc(AVA_CB *cb, AV
  
**/
  bool ava_B4_ver_used(AVA_CB *in_cb)
  {
-   AVA_CB *cb = 0;
 bool rc = false;

 if(in_cb) {
@@ -125,8 +124,7 @@ bool ava_B4_ver_used(AVA_CB *in_cb)
 rc = true;
 }
 else {
-
-   cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, 
gl_ava_hdl);
+   AVA_CB *cb = (AVA_CB 
*)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl);

 if(cb) {
 if((cb->version.releaseCode == 'B') && 
(cb->version.majorVersion == 0x04))
@@ -219,7 +217,7 @@ void amf_copy_from_SaAmfCallbacksT_4_to_
osaf_cbk->saAmfProxiedComponentInstantiateCallback = 
cbk->saAmfProxiedComponentInstantiateCallback;
osaf_cbk->saAmfProxiedComponentCleanupCallback = 
cbk->saAmfProxiedComponentCleanupCallback;
osaf_cbk->saAmfContainedComponentInstantiateCallback = 
cbk->saAmfContainedComponentInstantiateCallback;
-  osaf_cbk->saAmfContainedComponentInstantiateCallback = 
cbk->saAmfContainedComponentInstantiateCallback;
+  osaf_cbk->saAmfContainedComponentCleanupCallback = 
cbk->saAmfContainedComponentCleanupCallback;
  }

  /*
@@ -238,7 +236,6 @@ void amf_copy_from_SaAmfCallbacksT_o4_to
osaf_cbk->saAmfProxiedComponentInstantiateCallback = 
cbk->saAmfProxiedComponentInstantiateCallback;
osaf_cbk->saAmfProxiedComponentCleanupCallback = 
cbk->saAmfProxiedComponentCleanupCallback;
osaf_cbk->saAmfContainedComponentInstantiateCallback = 
cbk->saAmfContainedComponentInstantiateCallback;
-  osaf_cbk->saAmfContainedComponentInstantiateCallback = 
cbk->saAmfContainedComponentInstantiateCallback;
-  osaf_cbk->saAmfContainedComponentInstantiateCallback = 
cbk->saAmfContainedComponentInstantiateCallback;
-  osaf_cbk->osafCsiAttributeChangeCallback = 
cbk->osafCsiAttributeChangeCallback;
+  osaf_cbk->saAmfContainedComponentCleanupCallback = 
cbk->saAmfContainedComponentCleanupCallback;
+  osaf_cbk->osafCsiAttributeChangeCallback = 
cbk->osafCsiAttributeChangeCallback;
  }

===

-AVM

On 3/10/2017 1:12 PM, A V Mahesh wrote:
> Hi Gary,
>
> Thanks for the initial inputs.
>
> The patch doesn't contain any in-service /backward compatible change ,
> just wondering
> how this patch broken the  legacy Application , can you please
> code-snippet of your legacy Application,
> that breaking the patch.
>
> -AVM
>
> On 3/10/2017 10:59 AM, Gary Lee wrote:
>> Hi Mahesh
>>
>> Some of our legacy tests are failing after applying this series.
>>
>> I will do some debugging and send back details.
>>
>> Thanks
>> Gary
>>
>> -Original Message-
>> From: A V Mahesh 
>> Date: Wednesday, 8 March 2017 at 11:28 pm
>> To: , gary , 
>> , 
>> Cc: 
>> Subject: [PATCH 0 of 3] Review Request for amf: Fix all Cppcheck 1.77 issues 
>> [#2341] V1
>>
>>   Summary:amf: Fix all Cppcheck 1.77 issues [#2341] V1
>>   Review request for Trac Ticket(s): #2341
>>   Peer Reviewer(s): Amf Dev
>>   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 servicesy
>>Core libraries  n
>>Samples n
>>Tests   n
>>Other   n
>>   
>>   
>>   Comments (indicate scope for each "y" above):
>>   -
>>   
>>   changeset b8e7593cf4eadaa6169fe635ac0df67560ea875a
>>   Author:A V Mahesh 
>>   Date:  Wed, 08 Mar 2017 17:52:21 +0530
>>   
>>  amfd: Fix all Cppcheck 1.77 issues [#2341] V1
>>   
>>  [staging/src/amf/amfd/app.cc:285]: (style) The scope of the 
>> variable 'i' can
>>  be reduced. [staging/src/amf/amfd/apptype.cc:137]: (style) 
>> Condition 'rc!=0'
>>  is always false [staging/src/amf/amfd/apptype.cc:69]: (style) 
>> The scope of
>>  the variable