[devel] [PATCH 0 of 1] Review Request for amfd: correct logic in si dep flow [#276]
Summary: amfd: correct logic in si dep flow [#276] Review request for Trac Ticket(s): #276 Peer Reviewer(s): Praveen, Hans N, Minh, Gary Pull request to: LIST THE PERSON WITH PUSH ACCESS HERE 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 servicesy OpenSAF servicesn Core libraries n Samples n Tests n Other n Comments (indicate scope for each y above): - I am still testing it. I am floating it for early review because this looks only some logic correction and could be ok to review just the code. changeset 6ccb01038db382e520484eebfbbfe9b3b4bdc9c6 Author: Nagendra Kumarnagendr...@oracle.com Date: Wed, 26 Aug 2015 11:44:50 +0530 amfd: correct logic in si dep flow [#276] At many places, there has been tautological errors in si dep flow. The fix corrects them Complete diffstat: -- osaf/services/saf/amf/amfd/si_dep.cc | 25 - 1 files changed, 12 insertions(+), 13 deletions(-) Testing Commands: - Started system controller. Testing, Expected Results: -- No errors. 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. -- ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
[devel] [PATCH 1 of 1] amfd: correct logic in si dep flow [#276]
osaf/services/saf/amf/amfd/si_dep.cc | 25 -
1 files changed, 12 insertions(+), 13 deletions(-)
At many places, there has been tautological errors in si dep flow.
The fix corrects them
diff --git a/osaf/services/saf/amf/amfd/si_dep.cc
b/osaf/services/saf/amf/amfd/si_dep.cc
--- a/osaf/services/saf/amf/amfd/si_dep.cc
+++ b/osaf/services/saf/amf/amfd/si_dep.cc
@@ -1468,10 +1468,10 @@ void avd_sidep_unassign_dependents(AVD_S
if
(m_NCS_MDS_DEST_EQUAL(sisu-su-su_on_node-adest,su-su_on_node-adest)) {
avd_si_unassign(dep_si);
} else {
- if((dep_si-si_dep_state != AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN)) {
+ /* Don't start tol timer if dep state are either in
running or unassigned. */
+ if(!((dep_si-si_dep_state == AVD_SI_TOL_TIMER_RUNNING)
||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN))) {
avd_sidep_start_tolerance_timer_for_dependant(dep_si, si);
-
}
}
/* If this dependent SI is sponsor too, then unassign its
dependents also */
@@ -1788,9 +1788,9 @@ void avd_sidep_update_depstate_si_failov
if(su-su_on_node-saAmfNodeOperState ==
SA_AMF_OPERATIONAL_DISABLED) {
if
((m_NCS_MDS_DEST_EQUAL(sisu-su-su_on_node-adest,su-su_on_node-adest))) {
- if(((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state
!= AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state
!= AVD_SI_FAILOVER_UNDER_PROGRESS))
+ if((!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state
== AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state
== AVD_SI_FAILOVER_UNDER_PROGRESS)))
(avd_sidep_sponsors_assignment_states(dep_si))) {
avd_sidep_si_dep_state_set(dep_si, AVD_SI_FAILOVER_UNDER_PROGRESS);
@@ -1801,10 +1801,9 @@ void avd_sidep_update_depstate_si_failov
}
}
} else if (dep_si-sg_of_si == si-sg_of_si) {
- if((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state !=
AVD_SI_FAILOVER_UNDER_PROGRESS)) {
-
+ if(!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state ==
AVD_SI_FAILOVER_UNDER_PROGRESS))) {
avd_sidep_si_dep_state_set(dep_si, AVD_SI_FAILOVER_UNDER_PROGRESS);
if (dep_si-num_dependents 0)
{
/* This SI also has
dependents under it, update their state also */
@@ -1842,9 +1841,9 @@ void avd_sidep_update_depstate_si_failov
}
if (sponsor_assignments_state
== true) {
-
if((dep_si-si_dep_state != AVD_SI_TOL_TIMER_RUNNING) ||
-
(dep_si-si_dep_state != AVD_SI_READY_TO_UNASSIGN) ||
-
(dep_si-si_dep_state != AVD_SI_FAILOVER_UNDER_PROGRESS)) {
+
if(!((dep_si-si_dep_state == AVD_SI_TOL_TIMER_RUNNING) ||
+
(dep_si-si_dep_state == AVD_SI_READY_TO_UNASSIGN) ||
+
(dep_si-si_dep_state == AVD_SI_FAILOVER_UNDER_PROGRESS))) {
avd_sidep_si_dep_state_set(dep_si, AVD_SI_FAILOVER_UNDER_PROGRESS);
if
(dep_si-num_dependents 0) {
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
Re: [devel] [PATCH 1 of 1] amfd: correct logic in si dep flow [#276]
Code inside the if block was always executing, which means if condition
is not needed.
Why can't simply remove the if condition.
Thanks,
Praveen
On 26-Aug-15 11:51 AM, nagendr...@oracle.com wrote:
osaf/services/saf/amf/amfd/si_dep.cc | 25 -
1 files changed, 12 insertions(+), 13 deletions(-)
At many places, there has been tautological errors in si dep flow.
The fix corrects them
diff --git a/osaf/services/saf/amf/amfd/si_dep.cc
b/osaf/services/saf/amf/amfd/si_dep.cc
--- a/osaf/services/saf/amf/amfd/si_dep.cc
+++ b/osaf/services/saf/amf/amfd/si_dep.cc
@@ -1468,10 +1468,10 @@ void avd_sidep_unassign_dependents(AVD_S
if
(m_NCS_MDS_DEST_EQUAL(sisu-su-su_on_node-adest,su-su_on_node-adest)) {
avd_si_unassign(dep_si);
} else {
- if((dep_si-si_dep_state != AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN)) {
+ /* Don't start tol timer if dep state are either in
running or unassigned. */
+ if(!((dep_si-si_dep_state == AVD_SI_TOL_TIMER_RUNNING)
||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN))) {
avd_sidep_start_tolerance_timer_for_dependant(dep_si, si);
-
}
}
/* If this dependent SI is sponsor too, then unassign its
dependents also */
@@ -1788,9 +1788,9 @@ void avd_sidep_update_depstate_si_failov
if(su-su_on_node-saAmfNodeOperState ==
SA_AMF_OPERATIONAL_DISABLED) {
if
((m_NCS_MDS_DEST_EQUAL(sisu-su-su_on_node-adest,su-su_on_node-adest)))
{
- if(((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state
!= AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state
!= AVD_SI_FAILOVER_UNDER_PROGRESS))
+ if((!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state
== AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state
== AVD_SI_FAILOVER_UNDER_PROGRESS)))
(avd_sidep_sponsors_assignment_states(dep_si))) {
avd_sidep_si_dep_state_set(dep_si, AVD_SI_FAILOVER_UNDER_PROGRESS);
@@ -1801,10 +1801,9 @@ void avd_sidep_update_depstate_si_failov
}
}
} else if (dep_si-sg_of_si == si-sg_of_si) {
- if((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state !=
AVD_SI_FAILOVER_UNDER_PROGRESS)) {
-
+ if(!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state ==
AVD_SI_FAILOVER_UNDER_PROGRESS))) {
avd_sidep_si_dep_state_set(dep_si, AVD_SI_FAILOVER_UNDER_PROGRESS);
if (dep_si-num_dependents 0)
{
/* This SI also has
dependents under it, update their state also */
@@ -1842,9 +1841,9 @@ void avd_sidep_update_depstate_si_failov
}
if (sponsor_assignments_state
== true) {
-
if((dep_si-si_dep_state != AVD_SI_TOL_TIMER_RUNNING) ||
-
(dep_si-si_dep_state != AVD_SI_READY_TO_UNASSIGN) ||
-
(dep_si-si_dep_state != AVD_SI_FAILOVER_UNDER_PROGRESS)) {
+
if(!((dep_si-si_dep_state == AVD_SI_TOL_TIMER_RUNNING) ||
+
(dep_si-si_dep_state == AVD_SI_READY_TO_UNASSIGN) ||
+
(dep_si-si_dep_state == AVD_SI_FAILOVER_UNDER_PROGRESS))) {
avd_sidep_si_dep_state_set(dep_si, AVD_SI_FAILOVER_UNDER_PROGRESS);
if
Re: [devel] [PATCH 1 of 1] amfd: correct logic in si dep flow [#276]
Hi Praveen,
Thanks for your comment.
I think we can have two approaches:
1. Remove the code itself as suggested by you. OR
2. Correct the logic in the intended way. For example, please check below
correction:
- if((dep_si-si_dep_state != AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN)) {
+ /* Don't start tol timer if dep state are either in
running or unassigned. */
+ if(!((dep_si-si_dep_state == AVD_SI_TOL_TIMER_RUNNING)
||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN))) {
avd_sidep_start_tolerance_timer_for_dependant(dep_si, si);
The intention looks that if si_dep_state is in either tol_running or
ready_to_unassigned, then don't start the timer, else start the timer.
So, I corrected the logic in the intended way. Now, the code will have the same
sense as thought while coding.
And if the logic written is wrong in the first place and since the check was
always passing, now after
correction of code, the test case may fail. But then we can correct the fault
in si dep flow and add logic on top.
Let me know everybody opinion.
Thanks
-Nagu
-Original Message-
From: praveen malviya
Sent: 26 August 2015 12:15
To: Nagendra Kumar; hans.nordeb...@ericsson.com;
minh.c...@dektech.com.au; gary@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1 of 1] amfd: correct logic in si dep flow [#276]
Code inside the if block was always executing, which means if condition is not
needed.
Why can't simply remove the if condition.
Thanks,
Praveen
On 26-Aug-15 11:51 AM, nagendr...@oracle.com wrote:
osaf/services/saf/amf/amfd/si_dep.cc | 25 -
1 files changed, 12 insertions(+), 13 deletions(-)
At many places, there has been tautological errors in si dep flow.
The fix corrects them
diff --git a/osaf/services/saf/amf/amfd/si_dep.cc
b/osaf/services/saf/amf/amfd/si_dep.cc
--- a/osaf/services/saf/amf/amfd/si_dep.cc
+++ b/osaf/services/saf/amf/amfd/si_dep.cc
@@ -1468,10 +1468,10 @@ void avd_sidep_unassign_dependents(AVD_S
if (m_NCS_MDS_DEST_EQUAL(sisu-su-su_on_node-
adest,su-su_on_node-adest)) {
avd_si_unassign(dep_si);
} else {
- if((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN)) {
+ /* Don't start tol timer if dep state are either in
running or unassigned. */
+ if(!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN))) {
avd_sidep_start_tolerance_timer_for_dependant(dep_si, si);
-
}
}
/* If this dependent SI is sponsor too, then unassign its
dependents also */ @@ -1788,9 +1788,9 @@ void
avd_sidep_update_depstate_si_failov
if(su-su_on_node-saAmfNodeOperState ==
SA_AMF_OPERATIONAL_DISABLED) {
if ((m_NCS_MDS_DEST_EQUAL(sisu-
su-su_on_node-adest,su-su_on_node-adest))) {
- if(((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state
!= AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state
!= AVD_SI_FAILOVER_UNDER_PROGRESS))
+ if((!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state
== AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state
== AVD_SI_FAILOVER_UNDER_PROGRESS)))
(avd_sidep_sponsors_assignment_states(dep_si))) {
avd_sidep_si_dep_state_set(dep_si,
AVD_SI_FAILOVER_UNDER_PROGRESS); @@ -1801,10 +1801,9 @@ void
avd_sidep_update_depstate_si_failov
}
}
} else if (dep_si-sg_of_si == si-sg_of_si) {
- if((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state !=
AVD_SI_FAILOVER_UNDER_PROGRESS)) {
-
+ if(!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state ==
[devel] [PATCH 1 of 1] mds: fix memory leak in MDS auth server [#1462]
osaf/libs/core/mds/mds_main.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
Free allocated memory for MDS_PROCESS_INFO structure when
mds_register_callback() receives MDS_UNREGISTER_REQ.
diff --git a/osaf/libs/core/mds/mds_main.c b/osaf/libs/core/mds/mds_main.c
--- a/osaf/libs/core/mds/mds_main.c
+++ b/osaf/libs/core/mds/mds_main.c
@@ -191,6 +191,7 @@ static void mds_register_callback(int fd
MDS_PROCESS_INFO *info = mds_process_info_get(mds_dest, svc_id);
if (info != NULL) {
(void)mds_process_info_del(info);
+ free(info);
}
osaf_mutex_unlock_ordie(gl_mds_library_mutex);
--
___
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 mds: fix memory leak in MDS auth server [#1462]
Summary: mds: fix memory leak in MDS auth server [#1462] Review request for Trac Ticket(s): 1462 Peer Reviewer(s): Mahesh Pull request to: Zoran Affected branch(es): opensaf-4.5.x, opensaf-4.6.x, default(4.7) Development branch: default(4.7) 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 83bbe59386c0079a45b3f4ea10c60e4a63026a52 Author: Zoran Milinkovic zoran.milinko...@ericsson.com Date: Wed, 26 Aug 2015 17:45:03 +0200 mds: fix memory leak in MDS auth server [#1462] Free allocated memory for MDS_PROCESS_INFO structure when mds_register_callback() receives MDS_UNREGISTER_REQ. Complete diffstat: -- osaf/libs/core/mds/mds_main.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) Testing Commands: - Testing, Expected Results: -- After applying the patch, check with valgrind that there is no memory leak in mds_register_callback function. Conditions of Submission: - Ack from Mahesh 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. -- ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 1 of 1] amfd: Convert AVD_AVND list_of_ncs_su and list_of_ncs_su to std::vector [#1142]
I added my comments below.
/Thanks HansN
-Original Message-
From: Gary Lee [mailto:gary@dektech.com.au]
Sent: den 26 augusti 2015 07:41
To: Hans Nordebäck; praveen.malv...@oracle.com; nagendr...@oracle.com
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1 of 1] amfd: Convert AVD_AVND list_of_ncs_su and
list_of_ncs_su to std::vector [#1142]
ack (not tested) - two minor comments below.
On 21/08/15 23:22, Hans Nordeback wrote:
osaf/services/saf/amf/amfd/ckpt_dec.cc |2 +-
osaf/services/saf/amf/amfd/clm.cc| 12 +-
osaf/services/saf/amf/amfd/include/node.h| 27 +---
osaf/services/saf/amf/amfd/include/su.h |1 -
osaf/services/saf/amf/amfd/main.cc |3 +-
osaf/services/saf/amf/amfd/ndfsm.cc | 15 +-
osaf/services/saf/amf/amfd/ndproc.cc |7 +-
osaf/services/saf/amf/amfd/node.cc | 161
+-
osaf/services/saf/amf/amfd/nodegroup.cc | 31 ++---
osaf/services/saf/amf/amfd/nodeswbundle.cc |5 +-
osaf/services/saf/amf/amfd/role.cc |6 +-
osaf/services/saf/amf/amfd/sg_2n_fsm.cc | 21 +--
osaf/services/saf/amf/amfd/sg_nored_fsm.cc | 27 +---
osaf/services/saf/amf/amfd/sg_npm_fsm.cc | 28 +---
osaf/services/saf/amf/amfd/sg_nway_fsm.cc| 39 ++
osaf/services/saf/amf/amfd/sg_nwayact_fsm.cc | 25 +--
osaf/services/saf/amf/amfd/sgproc.cc | 56 ++--
osaf/services/saf/amf/amfd/su.cc |5 +-
osaf/services/saf/amf/amfd/util.cc |4 +-
19 files changed, 152 insertions(+), 323 deletions(-)
diff --git a/osaf/services/saf/amf/amfd/ckpt_dec.cc
b/osaf/services/saf/amf/amfd/ckpt_dec.cc
--- a/osaf/services/saf/amf/amfd/ckpt_dec.cc
+++ b/osaf/services/saf/amf/amfd/ckpt_dec.cc
@@ -3043,7 +3043,7 @@ static uint32_t dec_ng_admin_state(AVD_C
AVD_AVND *node = avd_node_get(*iter);
AVD_SU *su = NULL;
//If this node has any susi on it.
- for (su = node-list_of_su; su; su = su-avnd_list_su_next)
+ for (const auto su : node-list_of_su)
if (su-list_of_susi != NULL)
break;
if ((ng-saAmfNGAdminState == SA_AMF_ADMIN_SHUTTING_DOWN)
(su != NULL))
diff --git a/osaf/services/saf/amf/amfd/clm.cc
b/osaf/services/saf/amf/amfd/clm.cc
--- a/osaf/services/saf/amf/amfd/clm.cc
+++ b/osaf/services/saf/amf/amfd/clm.cc
@@ -26,8 +26,6 @@ static SaVersionT clmVersion = { 'B', 4,
static void clm_node_join_complete(AVD_AVND *node)
{
- AVD_SU *su;
-
TRACE_ENTER();
/* For each of the SUs calculate the readiness state.
** call the SG FSM with the new readiness state.
@@ -39,8 +37,7 @@ static void clm_node_join_complete(AVD_A
}
avd_node_oper_state_set(node, SA_AMF_OPERATIONAL_ENABLED);
- su = node-list_of_su;
- while (su != NULL) {
+ for (const auto su : node-list_of_su) {
/* For non-preinstantiable SU unlock-inst will not lead to its
inst until unlock. */
if ( su-saAmfSUPreInstantiable == false ) {
/* Skip the instantiation. */
@@ -68,8 +65,6 @@ static void clm_node_join_complete(AVD_A
}
}
}
- /* get the next SU on the node */
- su = su-avnd_list_su_next;
}
node_reset_su_try_inst_counter(node);
@@ -82,7 +77,6 @@ done:
/* validating this node for a graceful exit */
static void clm_node_exit_validate(AVD_AVND *node)
{
- AVD_SU *su;
AVD_SU_SI_REL *susi;
bool reject = false;
SaAisErrorT rc = SA_AIS_OK;
@@ -99,8 +93,7 @@ static void clm_node_exit_validate(AVD_A
/* now go through each SU to determine whether
any SI assigned becomes unassigned due to node exit*/
- su = node-list_of_su;
- while (su != NULL) {
+ for (const auto su : node-list_of_su) {
susi = su-list_of_susi;
/* now evalutate each SI that is assigned to this SU */
while (susi != NULL) {
@@ -114,7 +107,6 @@ static void clm_node_exit_validate(AVD_A
}
susi = susi-su_next;
}
- su = su-avnd_list_su_next;
}
done:
diff --git a/osaf/services/saf/amf/amfd/include/node.h
b/osaf/services/saf/amf/amfd/include/node.h
--- a/osaf/services/saf/amf/amfd/include/node.h
+++ b/osaf/services/saf/amf/amfd/include/node.h
@@ -43,6 +43,7 @@
#include timer.h
#include db_template.h
#include set
+#include vector
class AVD_SU;
struct avd_cluster_tag;
@@ -76,6 +77,8 @@ class AVD_AVND {
public:
AVD_AVND();
explicit AVD_AVND(const SaNameT* dn);
+
+ bool is_node_lock();
SaNameT name; /* DN */
char *node_name;/* RDN value,
[devel] [PATCH 1 of 1] mds: Optimized the mds mutex locks code [#1338]
osaf/libs/core/mds/mds_c_sndrcv.c | 40 +-
1 files changed, 2 insertions(+), 38 deletions(-)
Made centralized locks unlock wherever possible,
for better debugging mds code and to reduces scope of locks unlock code errors
diff --git a/osaf/libs/core/mds/mds_c_sndrcv.c
b/osaf/libs/core/mds/mds_c_sndrcv.c
--- a/osaf/libs/core/mds/mds_c_sndrcv.c
+++ b/osaf/libs/core/mds/mds_c_sndrcv.c
@@ -1804,7 +1804,6 @@ static uint32_t mds_subtn_tbl_add_disc_q
/* Now wait till the timeout or an subscription result will come */
- osaf_mutex_unlock_ordie(gl_mds_library_mutex);
switch (req-i_sendtype) {
case MDS_SENDTYPE_SND:
@@ -1888,7 +1887,6 @@ static uint32_t mds_subtn_tbl_add_disc_q
break;
}
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS != mds_check_for_mds_existence(add_ptr-sel_obj,
env_hdl, fr_svc_id, req-i_to_svc)) {
m_MDS_LOG_ERR(MDS_SND_RCV: MDS entry doesnt exist\n);
@@ -2465,9 +2463,7 @@ static uint32_t mcm_pvt_normal_svc_sndrs
get_svc_names(fr_svc_id), fr_svc_id,
get_svc_names(to_svc_id), to_svc_id, to_dest);
return status;
} else {
- osaf_mutex_unlock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS != mds_mcm_time_wait(sync_queue-sel_obj,
req-info.sndrsp.i_time_to_wait)) {
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
/* This is for response for local dest */
if (sync_queue-status == NCSCC_RC_SUCCESS) {
/* sucess case */
@@ -2488,7 +2484,6 @@ static uint32_t mcm_pvt_normal_svc_sndrs
mcm_pvt_del_sync_send_entry((MDS_PWE_HDL)env_hdl,
fr_svc_id, xch_id, req-i_sendtype, 0);
return NCSCC_RC_REQ_TIMOUT;
} else {
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS !=
mds_check_for_mds_existence(sync_queue-sel_obj, env_hdl, fr_svc_id,
to_svc_id)) {
m_MDS_LOG_INFO(MDS_SND_RCV: MDS entry doesnt
exist\n);
@@ -2579,10 +2574,12 @@ static uint32_t mds_await_active_tbl_del
static uint32_t mds_mcm_time_wait(NCS_SEL_OBJ *sel_obj, uint32_t time_val)
{
+ osaf_mutex_unlock_ordie(gl_mds_library_mutex);
/* Now wait for the response to come */
int count = osaf_poll_one_fd(sel_obj-rmv_obj,
time_val == 0 ? -1 : (time_val * 10));
+ osaf_mutex_lock_ordie(gl_mds_library_mutex);
if ((count == 0) || (count == -1)) {
/* Both for Timeout and Error Case */
m_MDS_LOG_ERR(MDS_SND_RCV: Timeout or Error occured\n);
@@ -2819,10 +2816,8 @@ static uint32_t mcm_pvt_normal_svc_sndra
m_MDS_ERR_PRINT_ANCHOR(msg_dest_adest);
return status;
} else {
- osaf_mutex_unlock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS != mds_mcm_time_wait(sync_queue-sel_obj,
req-info.sndrack.i_time_to_wait)) {
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
if (sync_queue-status == NCSCC_RC_SUCCESS) {
/* for local case */
/* sucess case */
@@ -2841,7 +2836,6 @@ static uint32_t mcm_pvt_normal_svc_sndra
msg_dest_adest);
return NCSCC_RC_REQ_TIMOUT;
} else {
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS !=
mds_check_for_mds_existence(sync_queue-sel_obj, env_hdl, fr_svc_id,
to_svc_id)) {
m_MDS_LOG_ERR(MDS_SND_RCV: MDS entry doesnt
exist\n);
@@ -3057,9 +3051,7 @@ static uint32_t mcm_pvt_normal_svc_sndac
m_MDS_ERR_PRINT_ADEST(to_dest);
return status;
} else {
- osaf_mutex_unlock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS != mds_mcm_time_wait(sync_queue-sel_obj,
req-info.sndack.i_time_to_wait)) {
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
if (sync_queue-status == NCSCC_RC_SUCCESS) {
/* sucess case */
mcm_pvt_del_sync_send_entry((MDS_PWE_HDL)env_hdl, fr_svc_id, xch_id,
req-i_sendtype,
@@ -3076,7 +3068,6 @@ static uint32_t mcm_pvt_normal_svc_sndac
mcm_pvt_del_sync_send_entry((MDS_PWE_HDL)env_hdl,
fr_svc_id, xch_id, req-i_sendtype, 0);
return NCSCC_RC_REQ_TIMOUT;
} else {
- osaf_mutex_lock_ordie(gl_mds_library_mutex);
if (NCSCC_RC_SUCCESS !=
mds_check_for_mds_existence(sync_queue-sel_obj, env_hdl, fr_svc_id,
to_svc_id)) {
[devel] [PATCH 0 of 1] Review Request for mds: Optimized the mds mutex locks code [#1338]
Summary:mds: Optimized the mds mutex locks code [#1338] Review request for Trac Ticket(s): #1338 Peer Reviewer(s): Nagu/Ramesh Pull request to: LIST THE PERSON WITH PUSH ACCESS HERE 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 servicesn Core libraries y Samples n Tests n Other n Comments (indicate scope for each y above): - EXPLAIN/COMMENT THE PATCH SERIES HERE changeset fa70106bcfe3778697c95ab87ad1c3b6e9dfe1fd Author: A V Mahesh mahesh.va...@oracle.com Date: Wed, 26 Aug 2015 14:06:15 +0530 mds: Optimized the mds mutex locks code [#1338] Made centralized locks unlock wherever possible, for better debugging mds code and to reduces scope of locks unlock code errors Complete diffstat: -- osaf/libs/core/mds/mds_c_sndrcv.c | 40 ++-- 1 files changed, 2 insertions(+), 38 deletions(-) Testing Commands: - Testing, Expected Results: -- 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. -- ___ 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 osaf: fix memory leak in daemonize [#1461]
Summary: osaf: fix memory leak in daemonize [#1461] Review request for Trac Ticket(s): 1461 Peer Reviewer(s): Tai Dinh, Ramesh Pull request to: Zoran Affected branch(es): opensaf-4.6.x, default(4.7) Development branch: default(4.7) 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 6965f5d2535817425236316cd9339db79b750043 Author: Zoran Milinkovic zoran.milinko...@ericsson.com Date: Wed, 26 Aug 2015 17:17:59 +0200 osaf: fix memory leak in daemonize [#1461] The patch fixes a memory leak adding endgrent() in missed places in osaf_get_group_list(). Complete diffstat: -- osaf/libs/core/common/osaf_secutil.c | 14 -- 1 files changed, 8 insertions(+), 6 deletions(-) Testing Commands: - Testing, Expected Results: -- Check with valgrind that there is no memory leak in daemonizing processes Conditions of Submission: - Ack from Ramesh and Tai Dinh 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. -- ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
[devel] [PATCH 1 of 1] osaf: fix memory leak in daemonize [#1461]
osaf/libs/core/common/osaf_secutil.c | 14 --
1 files changed, 8 insertions(+), 6 deletions(-)
The patch fixes a memory leak adding endgrent() in missed places in
osaf_get_group_list().
diff --git a/osaf/libs/core/common/osaf_secutil.c
b/osaf/libs/core/common/osaf_secutil.c
--- a/osaf/libs/core/common/osaf_secutil.c
+++ b/osaf/libs/core/common/osaf_secutil.c
@@ -327,7 +327,7 @@ done:
int osaf_get_group_list(const uid_t uid, const gid_t gid, gid_t *groups, int
*ngroups)
{
- int rc = 0;
+ int rc = -1;// Initially set to error
int size = 0;
int max_groups = sysconf(_SC_NGROUPS_MAX);
if (max_groups == -1){
@@ -370,7 +370,7 @@ int osaf_get_group_list(const uid_t uid,
struct group *gr = getgrent();
if (errno != 0) {
LOG_NO(setgrent failed: %s, strerror(errno));
- return -1;
+ goto done;
}
while (gr){
@@ -379,7 +379,7 @@ int osaf_get_group_list(const uid_t uid,
gr = getgrent();
if (errno != 0) {
LOG_NO(setgrent failed: %s, strerror(errno));
- return -1;
+ goto done;
}
continue;
}
@@ -399,12 +399,11 @@ int osaf_get_group_list(const uid_t uid,
gr = getgrent();
if (errno != 0) {
LOG_NO(setgrent failed: %s, strerror(errno));
- return -1;
+ goto done;
}
}
- endgrent();
-
+ // rc will be set to non-error value
if (groups){
*ngroups = (size *ngroups)? size : *ngroups;
rc = size;
@@ -413,5 +412,8 @@ int osaf_get_group_list(const uid_t uid,
rc = 0;
}
+done:
+ endgrent();
+
return rc;
}
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 1 of 1] mds: fix memory leak in MDS auth server [#1462]
Hi Zoran,
Dose valgrind report any thing at below code (mds_dt_common.c) as well ?
we have (void)mds_process_info_del(info) as well.
case MDS_DOWN_TMR: {
MDS_PROCESS_INFO *info =
mds_process_info_get(
tmr_req_info-info.down_event_tmr_info.adest,
tmr_req_info-info.down_event_tmr_info.svc_id);
/* only delete if process not exist to
avoid race with a client
* that re-registers immediately after
unregister */
if ((info != NULL) (kill(info-pid,
0) == -1)) {
TRACE(TIMEOUT, deleting entry
for %PRIx64, pid:%d,
info-mds_dest, info-pid);
(void)mds_process_info_del(info);
}
break;
-AVM
On 8/26/2015 9:19 PM, Zoran Milinkovic wrote:
osaf/libs/core/mds/mds_main.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
Free allocated memory for MDS_PROCESS_INFO structure when
mds_register_callback() receives MDS_UNREGISTER_REQ.
diff --git a/osaf/libs/core/mds/mds_main.c b/osaf/libs/core/mds/mds_main.c
--- a/osaf/libs/core/mds/mds_main.c
+++ b/osaf/libs/core/mds/mds_main.c
@@ -191,6 +191,7 @@ static void mds_register_callback(int fd
MDS_PROCESS_INFO *info = mds_process_info_get(mds_dest, svc_id);
if (info != NULL) {
(void)mds_process_info_del(info);
+ free(info);
}
osaf_mutex_unlock_ordie(gl_mds_library_mutex);
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 1 of 1] amfd: correct logic in si dep flow [#276]
Hi,
I agree with 2., we should correct the logic as it was intended. Btw,
ack for the patch.
Thanks,
Minh
On 8/26/2015 5:05 PM, Nagendra Kumar wrote:
Hi Praveen,
Thanks for your comment.
I think we can have two approaches:
1.Remove the code itself as suggested by you. OR
2.Correct the logic in the intended way. For example, please check below
correction:
-if((dep_si-si_dep_state != AVD_SI_TOL_TIMER_RUNNING) ||
-(dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN)) {
+/* Don't start tol timer if dep state are either in
running or unassigned. */
+if(!((dep_si-si_dep_state == AVD_SI_TOL_TIMER_RUNNING)
||
+(dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN))) {
avd_sidep_start_tolerance_timer_for_dependant(dep_si, si);
The intention looks that if si_dep_state is in either tol_running or
ready_to_unassigned, then don't start the timer, else start the timer.
So, I corrected the logic in the intended way. Now, the code will have the
same sense as thought while coding.
And if the logic written is wrong in the first place and since the check was
always passing, now after
correction of code, the test case may fail. But then we can correct the fault
in si dep flow and add logic on top.
Let me know everybody opinion.
Thanks
-Nagu
-Original Message-
From: praveen malviya
Sent: 26 August 2015 12:15
To: Nagendra Kumar; hans.nordeb...@ericsson.com;
minh.c...@dektech.com.au; gary@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1 of 1] amfd: correct logic in si dep flow [#276]
Code inside the if block was always executing, which means if condition is
not
needed.
Why can't simply remove the if condition.
Thanks,
Praveen
On 26-Aug-15 11:51 AM, nagendr...@oracle.com wrote:
osaf/services/saf/amf/amfd/si_dep.cc | 25 -
1 files changed, 12 insertions(+), 13 deletions(-)
At many places, there has been tautological errors in si dep flow.
The fix corrects them
diff --git a/osaf/services/saf/amf/amfd/si_dep.cc
b/osaf/services/saf/amf/amfd/si_dep.cc
--- a/osaf/services/saf/amf/amfd/si_dep.cc
+++ b/osaf/services/saf/amf/amfd/si_dep.cc
@@ -1468,10 +1468,10 @@ void avd_sidep_unassign_dependents(AVD_S
if (m_NCS_MDS_DEST_EQUAL(sisu-su-su_on_node-
adest,su-su_on_node-adest)) {
avd_si_unassign(dep_si);
} else {
- if((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN)) {
+ /* Don't start tol timer if dep state are either in
running or unassigned. */
+ if(!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state ==
AVD_SI_READY_TO_UNASSIGN))) {
avd_sidep_start_tolerance_timer_for_dependant(dep_si, si);
-
}
}
/* If this dependent SI is sponsor too, then unassign its
dependents also */ @@ -1788,9 +1788,9 @@ void
avd_sidep_update_depstate_si_failov
if(su-su_on_node-saAmfNodeOperState ==
SA_AMF_OPERATIONAL_DISABLED) {
if ((m_NCS_MDS_DEST_EQUAL(sisu-
su-su_on_node-adest,su-su_on_node-adest))) {
- if(((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state
!= AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state
!= AVD_SI_FAILOVER_UNDER_PROGRESS))
+ if((!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state
== AVD_SI_READY_TO_UNASSIGN) ||
+ (dep_si-si_dep_state
== AVD_SI_FAILOVER_UNDER_PROGRESS)))
(avd_sidep_sponsors_assignment_states(dep_si))) {
avd_sidep_si_dep_state_set(dep_si,
AVD_SI_FAILOVER_UNDER_PROGRESS); @@ -1801,10 +1801,9 @@ void
avd_sidep_update_depstate_si_failov
}
}
} else if (dep_si-sg_of_si == si-sg_of_si) {
- if((dep_si-si_dep_state !=
AVD_SI_TOL_TIMER_RUNNING) ||
- (dep_si-si_dep_state !=
AVD_SI_READY_TO_UNASSIGN) ||
- (dep_si-si_dep_state !=
AVD_SI_FAILOVER_UNDER_PROGRESS)) {
-
+ if(!((dep_si-si_dep_state ==
AVD_SI_TOL_TIMER_RUNNING) ||
+ (dep_si-si_dep_state ==
