[devel] [PATCH 0 of 1] Review Request for imm: Add missing checks for SaStringT attributes with ATTR_DN flag [#2270]

[Hung Nguyen]

Summary: imm: Add missing checks for SaStringT attributes with ATTR_DN flag 
[#2270]
Review request for Trac Ticket(s): 2270
Peer Reviewer(s): Zoran, Neel
Pull request to:
Affected branch(es): 5.0, 5.1, 5.2
Development branch: 5.2


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 d5a580a0c1b2d7ce1e0f91feb4e3021b4c416731
Author: Hung Nguyen 
Date:   Mon, 23 Jan 2017 14:52:21 +0700

imm: Add missing checks for SaStringT attributes with ATTR_DN flag 
[#2270]

ImmModel::notCompatibleAtt() Check for dangling values when adding
NO_DANGLING flag to SaStringDN attributes.

ImmModel::ccbObjectModify() Check for SaStringDN attributes that have 
long
DN values before disabling long DN (setting longDnsAllowed to 0).

ImmModel::rtObjectCreate() When long DN is disabled, don't allow 
creating
runtime objects with SaStringDN attributes that have long DN value.

ImmModel::rtObjectUpdate() When long DN is disabled, don't allow setting
long DN value to SaStringDN attributes.


Complete diffstat:
--
 src/imm/immnd/ImmModel.cc |  94 
--
 1 files changed, 52 insertions(+), 42 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.


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

[devel] [PATCH 1 of 1] imm: Add missing checks for SaStringT attributes with ATTR_DN flag [#2270]

[Hung Nguyen]

 src/imm/immnd/ImmModel.cc |  94 ++-
 1 files changed, 52 insertions(+), 42 deletions(-)


ImmModel::notCompatibleAtt()
Check for dangling values when adding NO_DANGLING flag to SaStringDN attributes.

ImmModel::ccbObjectModify()
Check for SaStringDN attributes that have long DN values before disabling long 
DN (setting longDnsAllowed to 0).

ImmModel::rtObjectCreate()
When long DN is disabled, don't allow creating runtime objects with SaStringDN 
attributes that have long DN value.

ImmModel::rtObjectUpdate()
When long DN is disabled, don't allow setting long DN value to SaStringDN 
attributes.

diff --git a/src/imm/immnd/ImmModel.cc b/src/imm/immnd/ImmModel.cc
--- a/src/imm/immnd/ImmModel.cc
+++ b/src/imm/immnd/ImmModel.cc
@@ -4563,8 +4563,12 @@ ImmModel::notCompatibleAtt(const std::st
 
 /* "changedAttrs != NULL" ensures that this check is only for the 
schema update */
 if(checkNoDanglingRefs && changedAttrs) {
-if(newAttr->mValueType != SA_IMM_ATTR_SANAMET)
+if (newAttr->mValueType != SA_IMM_ATTR_SANAMET &&
+!(newAttr->mValueType == SA_IMM_ATTR_SASTRINGT && 
(newAttr->mFlags & SA_IMM_ATTR_DN))) {
+LOG_NO("Impossible upgrade, attribute %s:%s adds/removes 
SA_IMM_ATTR_NO_DANGLING flag, ",
+   className.c_str(), attName.c_str());
 return true;
+}
 
 ClassMap::iterator cmi = sClassMap.find(className);
 osafassert(cmi != sClassMap.end());
@@ -9779,8 +9783,9 @@ ImmModel::ccbObjectModify(const ImmsvOmC
 goto bypass_impl;
 }
 }
-} else if((i4->second->mValueType == 
SA_IMM_ATTR_SANAMET) && 
- !(i4->second->mFlags & 
SA_IMM_ATTR_NO_DANGLING))
+} else if ((i4->second->mValueType == 
SA_IMM_ATTR_SANAMET ||
+(i4->second->mValueType == 
SA_IMM_ATTR_SASTRINGT && (i4->second->mFlags & SA_IMM_ATTR_DN))) &&
+!(i4->second->mFlags & 
SA_IMM_ATTR_NO_DANGLING))
 {
 /* DN limit check for attribute that is:
not the RDN attribute (else branch here)
@@ -15894,7 +15899,7 @@ ImmModel::rtObjectCreate(struct ImmsvOmC
 objectName.append((const char*)attrValues->n.attrValue.val.x.buf, 
 strnlen((const char*)attrValues->n.attrValue.val.x.buf,
 (size_t)attrValues->n.attrValue.val.x.size));
-} else if (attrValues->n.attrValueType == SA_IMM_ATTR_SANAMET
+} else if ((attrValues->n.attrValueType == SA_IMM_ATTR_SANAMET || 
attrValues->n.attrValueType == SA_IMM_ATTR_SASTRINGT)
 && !longDnsPermitted) {
 AttrMap::iterator it = classInfo->mAttrMap.find(attrName);
 if(it == classInfo->mAttrMap.end()) {
@@ -15903,24 +15908,27 @@ ImmModel::rtObjectCreate(struct ImmsvOmC
 err = SA_AIS_ERR_INVALID_PARAM;
 goto rtObjectCreateExit;
 }
-if(attrValues->n.attrValue.val.x.size >= 
SA_MAX_UNEXTENDED_NAME_LENGTH) {
-LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long DN. "
-"Not allowed by IMM service or extended names are 
disabled",
-attrName.c_str());
-err = SA_AIS_ERR_NAME_TOO_LONG;
-goto rtObjectCreateExit;
-}
-
-IMMSV_EDU_ATTR_VAL_LIST *value = attrValues->n.attrMoreValues;
-while(value) {
-if(value->n.val.x.size >= SA_MAX_UNEXTENDED_NAME_LENGTH) {
+
+if (attrValues->n.attrValueType == SA_IMM_ATTR_SANAMET || 
(it->second->mFlags & SA_IMM_ATTR_DN)) {
+if(attrValues->n.attrValue.val.x.size >= 
SA_MAX_UNEXTENDED_NAME_LENGTH) {
 LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long DN. "
 "Not allowed by IMM service or extended names are 
disabled",
 attrName.c_str());
 err = SA_AIS_ERR_NAME_TOO_LONG;
 goto rtObjectCreateExit;
 }
-value = value->next;
+
+IMMSV_EDU_ATTR_VAL_LIST *value = attrValues->n.attrMoreValues;
+while(value) {
+if(value->n.val.x.size >= SA_MAX_UNEXTENDED_NAME_LENGTH) {
+LOG_NO("ERR_NAME_TOO_LONG: Attribute '%s' has long DN. 
"
+"Not allowed by IMM service or extended names are 
disabled",
+attrName.c_str());
+err = SA_AIS_ERR_NAME_TOO_LONG;
+goto rtObjectCreateExit;
+}
+  

Re: [devel] [PATCH 2 of 2] AMFND: Fix SC failover during headless sync before standby AMFD comes up [#2162]

[Nagendra Kumar]

The logs (Logs-tc.rar) attached in the ticket.

Thanks
-Nagu

> -Original Message-
> From: minh chau [mailto:minh.c...@dektech.com.au]
> Sent: 16 January 2017 05:47
> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> gary@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 2 of 2] AMFND: Fix SC failover during headless sync
> before standby AMFD comes up [#2162]
> 
> Hi Nagu,
> 
> I misunderstood your point, and now I get it.
> In my test I see it works as expected - SU2 becomes Act and no assignment
> for SU1 I guess in your test some how the cluster initiation timer has not
> been started on SC2 (new active), there could be a missing case in the patch.
> Could you please share me the trace?
> 
> Thanks,
> Minh
> 
> On 13/01/17 21:48, Nagendra Kumar wrote:
> > Hi Minh,
> > Please check my response inlined with [Nagu].
> >
> > Thanks
> > -Nagu
> >> -Original Message-
> >> From: minh chau [mailto:minh.c...@dektech.com.au]
> >> Sent: 13 January 2017 03:53
> >> To: Nagendra Kumar; hans.nordeb...@ericsson.com; Praveen Malviya;
> >> gary@dektech.com.au
> >> Cc: opensaf-devel@lists.sourceforge.net
> >> Subject: Re: [PATCH 2 of 2] AMFND: Fix SC failover during headless
> >> sync before standby AMFD comes up [#2162]
> >>
> >> Hi Nagu,
> >>
> >> Thanks for reviewing, please see comments inline.
> >>
> >> Thanks,
> >> Minh
> >>
> >> On 12/01/17 21:48, Nagendra Kumar wrote:
> >>> Hi Minh,
> >>>Though I am not able to simulate the problem, I tested as below:
> >>> 1. Start SC1, SC2, PL-3 and PL-4. Configure SU1 on PL-3 as Act and
> >>> SU2 on
> >> PL-4 as Standby.
> >>> 2. Stop SC1 and SC2 and then stop PL-3.
> >>> 3. Start SC-1 and SC-2. When SC-2 prints Cold sync complete, stop
> >>> SC1. SC2
> >> becomes Act.
> >> [M]: As SU1 is on PL3, SU2 is on PL4, and If PL-3 is stopped, then
> >> only
> >> SU2 has active assignment
> > [Nagu]: PL-3 is stopped in step #2.
> >>> In this case, SC-2 contains both SU1(Act) and SU2(Standby) assignments.
> >>> Ideally, SU2 assignments should have been Act and there shouldn't be
> >>> SU1
> >> assignment.
> >> [M]: This seems to be another test where SU1 and SU2 are hosted on
> >> SC2, then both SU1 and SU2 should get assignment
> > [Nagu]: I mean to say command 'amf-state siass' run on SC-1 displays both
> SU1 and SU2 assignments.
> >  SU1 and SU2 are hosted on PL-3 and PL-4 respectively.
> > This is similar test case, which is mentioned in the ticket?
> >>>
> >>
> safSISU=safSu=SU1\,safSg=AmfDemo_2N\,safApp=AmfDemo1,safSi=AmfDe
> >> mo,safApp=AmfDemo1
> >>>   saAmfSISUHAState=ACTIVE(1)
> >>>   saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> >>>
> >>
> safSISU=safSu=SU2\,safSg=AmfDemo_2N\,safApp=AmfDemo1,safSi=AmfDe
> >> mo,safApp=AmfDemo1
> >>>   saAmfSISUHAState=STANDBY(2)
> >>>   saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> >>>
> >>> Please check.
> >>>
> >>> Thanks
> >>> -Nagu
> >>>
>  -Original Message-
>  From: Minh Hon Chau [mailto:minh.c...@dektech.com.au]
>  Sent: 08 November 2016 08:53
>  To: hans.nordeb...@ericsson.com; Nagendra Kumar; Praveen Malviya;
>  gary@dektech.com.au; minh.c...@dektech.com.au
>  Cc: opensaf-devel@lists.sourceforge.net
>  Subject: [PATCH 2 of 2] AMFND: Fix SC failover during headless sync
>  before standby AMFD comes up [#2162]
> 
> osaf/services/saf/amf/amfnd/di.cc   |  7 +--
> osaf/services/saf/amf/amfnd/susm.cc |  6 ++
> 2 files changed, 11 insertions(+), 2 deletions(-)
> 
> 
>  This case of SC failover causes new active AMFD getting stuck in
>  node_up messages
> 
>  Say first active controller is SC1, which goes down during headless sync.
>  Therefore, the amfnd on SC2 receives mds_down of AVD, then both
>  is_avd_down and amfd_sync_required are set to true. When SC2 takes
>  over active role, amfnd on SC2 receives mds_up, but only
>  is_avd_down is set to false and the variable amfd_sync_required
> remains true.
>  When amfnd-SC2 finishes initiating middleware SU, it needs to send
>  su_oper message to AMFD, but it is failed to send out due to
> >> amfd_sync_required.
>  In this scenario of SC failover, amfd_sync_required needs to set to
>  false when amfnd on SC2 receives su_pres message on middleware
> SUs.
>  That means amfnd on active controller does not need to wait for
>  set_leds message, to be informed that cluster initiation is done,
>  so that amfnd can sen su_oper messages to AMFD. This logic also
>  aligns with normal headless scenario, where amfnd on active
>  controller has amfd_sync_required initially marked as false because
>  no middleware SUs are initiated. When amfd_sync_required is true
>  that means amfnd all middleware SUs are initiated and assigned
>  before headless, thus amfnd needs to wait for cluster initiation after

[devel] [PATCH 1 of 1] AMFND: Ensure su operational message synchronizes with component failover sequence [#2233]

[Minh Hon Chau]

 src/amf/amfnd/avnd_su.h |   1 +
 src/amf/amfnd/clc.cc|   3 ---
 src/amf/amfnd/di.cc |  12 +++-
 src/amf/amfnd/susm.cc   |  32 +---
 4 files changed, 41 insertions(+), 7 deletions(-)


In case component failover, faulty component will be terminated. When the 
reinstantiation
is done, amfnd will send su_oper_message (enabled) to amfd which is running 
along with
component failover. In the reported problem, if su_oper_message (enabled) comes 
to amfd
before the quiesced assignment response (as part of component failover 
sequence) comes to
amfd, then this quiesced assignment response is ignored, thus component 
failover will not
finish.

The problem is in function susi_success_sg_realign with act=5, state=3, amfd 
always assumes
su having faulty component is OUT_OF_SERVICE. This assumption is true in most 
of the time
when su_oper_message (enabled) comes a little later than quiesced assignment 
response. In fact
the su_oper_message (enabled) is not designed as part of component failover 
sequence, thus it
can come any time during the failover. If amfd is getting a bit busier with RTA 
update then
the faulty component has enough to reinstiantiate so that amfnd sends 
su_oper_message (enabled)
before quiesced assignment response, the reported problem will be seen.

This patch hardens the component failover sequence by ensuring the 
su_oper_message (enabled) to
be sent after su completes to remove assignment. This approach comes from the 
similarity in
su failover, where the su_oper_message (enabled) is sent in repair phase.

diff --git a/src/amf/amfnd/avnd_su.h b/src/amf/amfnd/avnd_su.h
--- a/src/amf/amfnd/avnd_su.h
+++ b/src/amf/amfnd/avnd_su.h
@@ -393,6 +393,7 @@ extern struct avnd_su_si_rec *avnd_silis
 extern struct avnd_su_si_rec *avnd_silist_getprev(const struct avnd_su_si_rec 
*);
 extern struct avnd_su_si_rec *avnd_silist_getlast(void);
 extern bool sufailover_in_progress(const AVND_SU *su);
+extern bool componentfailover_in_progress(const AVND_SU *su);
 extern bool sufailover_during_nodeswitchover(const AVND_SU *su);
 extern bool all_csis_in_removed_state(const AVND_SU *su);
 extern void su_reset_restart_count_in_comps(const struct avnd_cb_tag *cb, 
const AVND_SU *su);
diff --git a/src/amf/amfnd/clc.cc b/src/amf/amfnd/clc.cc
--- a/src/amf/amfnd/clc.cc
+++ b/src/amf/amfnd/clc.cc
@@ -2381,9 +2381,6 @@ uint32_t avnd_comp_clc_terming_cleansucc
(m_AVND_SU_IS_FAILOVER(su))) {
/* yes, request director to orchestrate component failover */
rc = avnd_di_oper_send(cb, su, SA_AMF_COMPONENT_FAILOVER);
-
-   //Reset component-failover here. SU failover is reset as part 
of REPAIRED admin op.
-   m_AVND_SU_FAILOVER_RESET(su);
}
 
/*
diff --git a/src/amf/amfnd/di.cc b/src/amf/amfnd/di.cc
--- a/src/amf/amfnd/di.cc
+++ b/src/amf/amfnd/di.cc
@@ -894,7 +894,17 @@ uint32_t avnd_di_susi_resp_send(AVND_CB 
}
m_AVND_SU_ALL_SI_RESET(su);
 }
-
+if (componentfailover_in_progress(su)) {
+   if (all_csis_in_removed_state(su) == true) {
+   bool is_en;
+   m_AVND_SU_IS_ENABLED(su, is_en);
+   if (is_en) {
+   if (avnd_di_oper_send(cb, su, 0) == 
NCSCC_RC_SUCCESS) {
+   m_AVND_SU_FAILOVER_RESET(su);
+   }
+   }
+   }
+}
/* free the contents of avnd message */
avnd_msg_content_free(cb, );
 
diff --git a/src/amf/amfnd/susm.cc b/src/amf/amfnd/susm.cc
--- a/src/amf/amfnd/susm.cc
+++ b/src/amf/amfnd/susm.cc
@@ -1633,10 +1633,22 @@ uint32_t avnd_su_pres_st_chng_prc(AVND_C
m_AVND_SU_IS_ENABLED(su, is_en);
if (true == is_en) {
TRACE("SU oper state is enabled");
+   // do not send su_oper state if component 
failover is in progress
m_AVND_SU_OPER_STATE_SET(su, 
SA_AMF_OPERATIONAL_ENABLED);
-   rc = avnd_di_oper_send(cb, su, 0);
-   if (NCSCC_RC_SUCCESS != rc)
-   goto done;
+   if (componentfailover_in_progress(su) == true) {
+   si = reinterpret_cast
+   
(m_NCS_DBLIST_FIND_FIRST(>si_list));
+   if (si == nullptr || 
all_csis_in_removed_state(su)) {
+   rc = avnd_di_oper_send(cb, su, 
0);
+   if (rc != NCSCC_RC_SUCCESS)
+   goto done;
+   

[devel] [PATCH 0 of 1] Review Request for AMFND: Ensure su operational message synchronizes with component failover sequence [#2233]

[Minh Hon Chau]

Summary: AMFND: Ensure su operational message synchronizes with component 
failover sequence [#2233]
Review request for Trac Ticket(s): 2233
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 092fac5ca734f001552e5d6abe18391aaec42bd0
Author: Minh Hon Chau 
Date:   Mon, 23 Jan 2017 12:08:46 +1100

AMFND: Ensure su operational message synchronizes with component 
failover
sequence [#2233]

In case component failover, faulty component will be terminated. When 
the
reinstantiation is done, amfnd will send su_oper_message (enabled) to 
amfd
which is running along with component failover. In the reported 
problem, if
su_oper_message (enabled) comes to amfd before the quiesced assignment
response (as part of component failover sequence) comes to amfd, then 
this
quiesced assignment response is ignored, thus component failover will 
not
finish.

The problem is in function susi_success_sg_realign with act=5, state=3, 
amfd
always assumes su having faulty component is OUT_OF_SERVICE. This 
assumption
is true in most of the time when su_oper_message (enabled) comes a 
little
later than quiesced assignment response. In fact the su_oper_message
(enabled) is not designed as part of component failover sequence, thus 
it
can come any time during the failover. If amfd is getting a bit busier 
with
RTA update then the faulty component has enough to reinstiantiate so 
that
amfnd sends su_oper_message (enabled) before quiesced assignment 
response,
the reported problem will be seen.

This patch hardens the component failover sequence by ensuring the
su_oper_message (enabled) to be sent after su completes to remove
assignment. This approach comes from the similarity in su failover, 
where
the su_oper_message (enabled) is sent in repair phase.


Complete diffstat:
--
 src/amf/amfnd/avnd_su.h |   1 +
 src/amf/amfnd/clc.cc|   3 ---
 src/amf/amfnd/di.cc |  12 +++-
 src/amf/amfnd/susm.cc   |  32 +---
 4 files changed, 41 insertions(+), 7 deletions(-)


Testing Commands:
-
 - Testing normal component failover
 - Testing the scenario reported in ticket
 - Testing component failover during headless


Testing, Expected Results:
--
 All tests should pass


Conditions of Submission:
-
 ack from reviewer


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