opensaf-devel — OpenSAF Development and Reviews

Re: [devel] [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

[Thang D. N. <tha...@de...>]

Looks good. Ack.

-----Original Message-----
From: Thien Minh Huynh <thi...@de...> 
Sent: Thursday, May 2, 2024 1:11 PM
To: Thang Duc Nguyen <tha...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li...; Thien Minh Huynh <thi...@de...>
Subject: [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

An earlier patch supporting Python 3 introduced the SaStringT class.
With str type, default SaStringT cannot be compared.

This commit is to allow compare with str class if without casting
---
 python/pyosaf/saAis.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/python/pyosaf/saAis.py b/python/pyosaf/saAis.py index 912523e69..2f2af641f 100644
--- a/python/pyosaf/saAis.py
+++ b/python/pyosaf/saAis.py
@@ -53,6 +53,30 @@ if PY3:
 				return self.value.decode('utf-8')
 			else:
 				return self.__repr__()
+
+		def __eq__(self, other):
+			if isinstance(other, str):
+				return self.value == other.encode('utf-8')
+			return super().__eq__(other)
+
+		def __ne__(self, other):
+			return not self.__eq__(other)
+
+		def __lt__(self, other):
+			if isinstance(other, str):
+				return self.value < other.encode('utf-8')
+			return super().__lt__(other)
+
+		def __gt__(self, other):
+			if isinstance(other, str):
+				return self.value > other.encode('utf-8')
+			return super().__gt__(other)
+
+		def __ge__(self, other):
+			return self.__gt__(other) or self.__eq__(other)
+
+		def __le__(self, other):
+			return self.__lt__(other) or self.__eq__(other)
 else:
 	SaStringT = ctypes.c_char_p
 
--
2.43.0

[devel] [PATCH 0/1] Review Request for pyosaf: Allow SaStringT compare with str by default [#3351] V2

[thien.m.huynh <thi...@de...>]

Summary: pyosaf: Allow SaStringT compare with str by default [#3351]
Review request for Ticket(s): 3351
Peer Reviewer(s): Thang, Dat
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3351
Base revision: 7b80b26c5ac19135820854a6edc6d50a5dea82dc
Personal repository: git://git.code.sf.net/u/thienhuynh/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            n
 OpenSAF services        n
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   y


Comments (indicate scope for each "y" above):
---------------------------------------------
*** EXPLAIN/COMMENT THE PATCH SERIES HERE ***

revision 321e1d9eeac01d5259c73c88cea56e34462ebe8e
Author:	thien.m.huynh <thi...@de...>
Date:	Thu, 2 May 2024 11:25:09 +0700

pyosaf: Allow SaStringT compare with str by default [#3351]

An earlier patch supporting Python 3 introduced the SaStringT class.
With str type, default SaStringT cannot be compared.

This commit is to allow compare with str class if without casting



Complete diffstat:
------------------
 python/pyosaf/saAis.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)


Testing Commands:
-----------------
N/A

Testing, Expected Results:
--------------------------
N/A

Conditions of Submission:
-------------------------
ACK from reviewers

Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.

[devel] [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

[thien.m.huynh <thi...@de...>]

An earlier patch supporting Python 3 introduced the SaStringT class.
With str type, default SaStringT cannot be compared.

This commit is to allow compare with str class if without casting
---
 python/pyosaf/saAis.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/python/pyosaf/saAis.py b/python/pyosaf/saAis.py
index 912523e69..2f2af641f 100644
--- a/python/pyosaf/saAis.py
+++ b/python/pyosaf/saAis.py
@@ -53,6 +53,30 @@ if PY3:
 				return self.value.decode('utf-8')
 			else:
 				return self.__repr__()
+
+		def __eq__(self, other):
+			if isinstance(other, str):
+				return self.value == other.encode('utf-8')
+			return super().__eq__(other)
+
+		def __ne__(self, other):
+			return not self.__eq__(other)
+
+		def __lt__(self, other):
+			if isinstance(other, str):
+				return self.value < other.encode('utf-8')
+			return super().__lt__(other)
+
+		def __gt__(self, other):
+			if isinstance(other, str):
+				return self.value > other.encode('utf-8')
+			return super().__gt__(other)
+
+		def __ge__(self, other):
+			return self.__gt__(other) or self.__eq__(other)
+
+		def __le__(self, other):
+			return self.__lt__(other) or self.__eq__(other)
 else:
 	SaStringT = ctypes.c_char_p
 
-- 
2.43.0

Re: [devel] [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

[Thien M. H. <thi...@de...>]

Hi Thang,

Thanks for your review. I will update in the next patch.

Best Regards,
Thien
________________________________
From: Thang Duc Nguyen <tha...@de...>
Sent: Thursday, April 25, 2024 7:50 AM
To: Thien Minh Huynh <thi...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li... <ope...@li...>
Subject: RE: [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

Hi Thien,
It's better if other operators can be applied, e.i, <, >, ....

B.R/Thang

-----Original Message-----
From: Thien Minh Huynh <thi...@de...>
Sent: Tuesday, April 23, 2024 10:45 AM
To: Thang Duc Nguyen <tha...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li...; Thien Minh Huynh <thi...@de...>
Subject: [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

SaStringT class is introduced in previous commit support python3.
Default SaStringT cannot compare with other type.

This commit is to allow compare with str class if without casting
---
 python/pyosaf/saAis.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/python/pyosaf/saAis.py b/python/pyosaf/saAis.py index 912523e69..d890c6a27 100644
--- a/python/pyosaf/saAis.py
+++ b/python/pyosaf/saAis.py
@@ -53,6 +53,16 @@ if PY3:
                                 return self.value.decode('utf-8')
                         else:
                                 return self.__repr__()
+
+               def __eq__(self, other):
+                       if isinstance(other, str):
+                               return str(self).__eq__(other)
+                       return super().__eq__(other)
+
+               def __ne__(self, other):
+                       if isinstance(other, str):
+                               return str(self).__ne__(other)
+                       return super().__ne__(other)
 else:
         SaStringT = ctypes.c_char_p

--
2.43.0

Re: [devel] [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

[Thang D. N. <tha...@de...>]

Hi Thien,
It's better if other operators can be applied, e.i, <, >, ....

B.R/Thang

-----Original Message-----
From: Thien Minh Huynh <thi...@de...> 
Sent: Tuesday, April 23, 2024 10:45 AM
To: Thang Duc Nguyen <tha...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li...; Thien Minh Huynh <thi...@de...>
Subject: [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

SaStringT class is introduced in previous commit support python3.
Default SaStringT cannot compare with other type.

This commit is to allow compare with str class if without casting
---
 python/pyosaf/saAis.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/python/pyosaf/saAis.py b/python/pyosaf/saAis.py index 912523e69..d890c6a27 100644
--- a/python/pyosaf/saAis.py
+++ b/python/pyosaf/saAis.py
@@ -53,6 +53,16 @@ if PY3:
 				return self.value.decode('utf-8')
 			else:
 				return self.__repr__()
+
+		def __eq__(self, other):
+			if isinstance(other, str):
+				return str(self).__eq__(other)
+			return super().__eq__(other)
+
+		def __ne__(self, other):
+			if isinstance(other, str):
+				return str(self).__ne__(other)
+			return super().__ne__(other)
 else:
 	SaStringT = ctypes.c_char_p
 
--
2.43.0

[devel] [PATCH 0/1] Review Request for pyosaf: Allow SaStringT compare with str by default [#3351]

[thien.m.huynh <thi...@de...>]

Summary: pyosaf: Allow SaStringT compare with str by default [#3351]
Review request for Ticket(s): 3351
Peer Reviewer(s): Thang, Dat
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3351
Base revision: 7b80b26c5ac19135820854a6edc6d50a5dea82dc
Personal repository: git://git.code.sf.net/u/thienhuynh/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            n
 OpenSAF services        n
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   y


Comments (indicate scope for each "y" above):
---------------------------------------------
*** EXPLAIN/COMMENT THE PATCH SERIES HERE ***

revision 2793a552e9f94419fc72d58ccb767c2e0ce7ebd6
Author:	thien.m.huynh <thi...@de...>
Date:	Tue, 23 Apr 2024 10:30:57 +0700

pyosaf: Allow SaStringT compare with str by default [#3351]

SaStringT class is introduced in previous commit support python3.
Default SaStringT cannot compare with other type.

This commit is to allow compare with str class if without casting



Complete diffstat:
------------------
 python/pyosaf/saAis.py | 10 ++++++++++
 1 file changed, 10 insertions(+)


Testing Commands:
-----------------
N/A

Testing, Expected Results:
--------------------------
N/A

Conditions of Submission:
-------------------------
ACK from reviewers


Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.

[devel] [PATCH 1/1] pyosaf: Allow SaStringT compare with str by default [#3351]

[thien.m.huynh <thi...@de...>]

SaStringT class is introduced in previous commit support python3.
Default SaStringT cannot compare with other type.

This commit is to allow compare with str class if without casting
---
 python/pyosaf/saAis.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/python/pyosaf/saAis.py b/python/pyosaf/saAis.py
index 912523e69..d890c6a27 100644
--- a/python/pyosaf/saAis.py
+++ b/python/pyosaf/saAis.py
@@ -53,6 +53,16 @@ if PY3:
 				return self.value.decode('utf-8')
 			else:
 				return self.__repr__()
+
+		def __eq__(self, other):
+			if isinstance(other, str):
+				return str(self).__eq__(other)
+			return super().__eq__(other)
+
+		def __ne__(self, other):
+			if isinstance(other, str):
+				return str(self).__ne__(other)
+			return super().__ne__(other)
 else:
 	SaStringT = ctypes.c_char_p
 
-- 
2.43.0

Re: [devel] [PATCH 1/1] ntf: check existed notification before retrieving [#3350]

[Thien M. H. <thi...@de...>]

Hi Dat,

ACK from me.

Best Regards,
Thien

________________________________
From: Dat Tran Quoc Phan <dat...@de...>
Sent: Monday, April 15, 2024 5:10 PM
To: Thang Duc Nguyen <tha...@de...>; Thien Minh Huynh <thi...@de...>; Tai Huynh Nguyen <tai...@de...>
Cc: ope...@li... <ope...@li...>; Dat Tran Quoc Phan <dat...@de...>
Subject: [PATCH 1/1] ntf: check existed notification before retrieving [#3350]

Related to ticket #3349, when ntf removes an overdue notification
from logger buffer, it will try to retrieve next notification
in queue. In case there's only a single notification in queue
and it's overdue, after removing it, ntf will try to retrieve
non-existed notification, causing ntf crash.

Solution is to check if there's existed notification in queue
before retrieving it. Also, improve checking existed notification
when logd calls saLogWriteCallback.
---
 src/ntf/ntfd/NtfLogger.cc | 8 +++++---
 src/ntf/ntfd/NtfLogger.h  | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/ntf/ntfd/NtfLogger.cc b/src/ntf/ntfd/NtfLogger.cc
index 2b5c501ee..480c9e7b6 100644
--- a/src/ntf/ntfd/NtfLogger.cc
+++ b/src/ntf/ntfd/NtfLogger.cc
@@ -108,7 +108,7 @@ void saLogStreamOpenCallback(SaInvocationT invocation,

 void saLogWriteLogCallback(SaInvocationT invocation, SaAisErrorT error) {
   TRACE_ENTER2("Callback for notificationId %llu", invocation);
-  if (NtfAdmin::theNtfAdmin->logger.getFrontNotificationId() != invocation) {
+  if (!NtfAdmin::theNtfAdmin->logger.isExistNotification(invocation)) {
     TRACE("Notification had been processed by logd, but Id is not existed"
           " in logger. Probably due to notification overdue, ignore "
           "notificationId %llu", invocation);
@@ -384,8 +384,9 @@ void NtfLogger::disableAckWaiting() {
   notification->setWaitingAck(false);
 }

-SaNtfIdentifierT NtfLogger::getFrontNotificationId() {
-  return (queuedNotificationList.front()->getNotificationId());
+bool NtfLogger::isExistNotification(SaInvocationT invocation) {
+  if (isLoggerBufferEmpty()) return false;
+  return (queuedNotificationList.front()->getNotificationId() == invocation);
 }

 void NtfLogger::logQueuedNotification() {
@@ -397,6 +398,7 @@ void NtfLogger::logQueuedNotification() {
       dequeueNotification();
       resetLoggerBufferFullFlag();
       sendLoggedConfirm(notification->getNotificationId());
+      if (isLoggerBufferEmpty()) return;
       notification = queuedNotificationList.front();
     }
     if (notification->isWaitingAck()) return;
diff --git a/src/ntf/ntfd/NtfLogger.h b/src/ntf/ntfd/NtfLogger.h
index 283fd3fe2..4af8fc3d8 100644
--- a/src/ntf/ntfd/NtfLogger.h
+++ b/src/ntf/ntfd/NtfLogger.h
@@ -73,7 +73,7 @@ class NtfLogger {
   void logQueuedNotification();
   bool isLoggerBufferEmpty() { return queuedNotificationList.empty(); }
   void disableAckWaiting();
-  SaNtfIdentifierT getFrontNotificationId();
+  bool isExistNotification(SaInvocationT invocation);

  private:
   SaAisErrorT initLog();
--
2.25.1

Re: [devel] [PATCH 1/1] ntf: check existed notification before retrieving [#3350]

[Thien M. H. <thi...@de...>]

Hi Dat,

See  my comment inline.

Best Regards,
Thien

________________________________
From: Dat Tran Quoc Phan <dat...@de...>
Sent: Monday, April 15, 2024 9:38 AM
To: Thang Duc Nguyen <tha...@de...>; Thien Minh Huynh <thi...@de...>; Tai Huynh Nguyen <tai...@de...>
Cc: ope...@li... <ope...@li...>; Dat Tran Quoc Phan <dat...@de...>
Subject: [PATCH 1/1] ntf: check existed notification before retrieving [#3350]

Related to ticket #3349, when ntf removes an overdue notification
from logger buffer, it will try to retrieve next notification
in queue. In case there's only a single notification in queue
and it's overdue, after removing it, ntf will try to retrieve
non-existed notification, causing ntf crash.

Solution is to check if there's existed notification
in queue before retrieving it. Also, checking existed
notification when logd calls saLogWriteCallback.
---
 src/ntf/ntfd/NtfLogger.cc | 8 +++++---
 src/ntf/ntfd/NtfLogger.h  | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/ntf/ntfd/NtfLogger.cc b/src/ntf/ntfd/NtfLogger.cc
index 2b5c501ee..a6ca87d75 100644
--- a/src/ntf/ntfd/NtfLogger.cc
+++ b/src/ntf/ntfd/NtfLogger.cc
@@ -108,7 +108,7 @@ void saLogStreamOpenCallback(SaInvocationT invocation,

 void saLogWriteLogCallback(SaInvocationT invocation, SaAisErrorT error) {
   TRACE_ENTER2("Callback for notificationId %llu", invocation);
-  if (NtfAdmin::theNtfAdmin->logger.getFrontNotificationId() != invocation) {
+  if (!NtfAdmin::theNtfAdmin->logger.isExistNotification(invocation)) {
     TRACE("Notification had been processed by logd, but Id is not existed"
           " in logger. Probably due to notification overdue, ignore "
           "notificationId %llu", invocation);
@@ -384,8 +384,9 @@ void NtfLogger::disableAckWaiting() {
   notification->setWaitingAck(false);
 }

-SaNtfIdentifierT NtfLogger::getFrontNotificationId() {
-  return (queuedNotificationList.front()->getNotificationId());
+bool NtfLogger::isExistNotification(SaInvocationT invocation) {
+  return (!isLoggerBufferEmpty() &&
+          (queuedNotificationList.front()->getNotificationId() == invocation));
[Thien]: I suggest break down condition to easy read
if (isLoggerBufferEmpty()) return false;
return queuedNotificationList.front()->getNotificationId() == invocation;
 }

 void NtfLogger::logQueuedNotification() {
@@ -397,6 +398,7 @@ void NtfLogger::logQueuedNotification() {
       dequeueNotification();
       resetLoggerBufferFullFlag();
       sendLoggedConfirm(notification->getNotificationId());
+      if (isLoggerBufferEmpty()) return;
       notification = queuedNotificationList.front();
     }
     if (notification->isWaitingAck()) return;
diff --git a/src/ntf/ntfd/NtfLogger.h b/src/ntf/ntfd/NtfLogger.h
index 283fd3fe2..4af8fc3d8 100644
--- a/src/ntf/ntfd/NtfLogger.h
+++ b/src/ntf/ntfd/NtfLogger.h
@@ -73,7 +73,7 @@ class NtfLogger {
   void logQueuedNotification();
   bool isLoggerBufferEmpty() { return queuedNotificationList.empty(); }
   void disableAckWaiting();
-  SaNtfIdentifierT getFrontNotificationId();
+  bool isExistNotification(SaInvocationT invocation);

  private:
   SaAisErrorT initLog();
--
2.25.1

Re: [devel] [PATCH 0/2] Review Request for ticket 3349 V3

[Thien M. H. <thi...@de...>]

Hi Dat,

Ack from me.

Best Regards,
Thien
________________________________
From: Dat Tran Quoc Phan <dat...@de...>
Sent: Tuesday, April 2, 2024 8:47 AM
To: Thang Duc Nguyen <tha...@de...>; Thien Minh Huynh <thi...@de...>; Tai Huynh Nguyen <tai...@de...>
Cc: ope...@li... <ope...@li...>; Dat Tran Quoc Phan <dat...@de...>
Subject: [PATCH 0/2] Review Request for ticket 3349 V3

Summary: ntf: remove overdue notification from logger buffer [#3349]
Review request for Ticket(s): 3349
Peer Reviewer(s): Thang, Thien, Tai
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3349
Base revision: fcb47390bf79f1b5026093c2b5456cc1d48ecb79
Personal repository: git://git.code.sf.net/u/dattqphan/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            n
 OpenSAF services        y
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   n


Comments (indicate scope for each "y" above):
---------------------------------------------
*** EXPLAIN/COMMENT THE PATCH SERIES HERE ***

revision ad744c90e242272fe78ba09694e41de5eb164455
Author: dat.tq.phan <dat...@de...>
Date:   Tue, 2 Apr 2024 07:50:57 +0700

amf: move ntf job to the end of queue [#3349]

When AMF sends job to NTF but NTF is busy with full buffer,
AMF will be stuck and unable to process other jobs.

Solution is to move the front NTF job to the end of queue
when NTF is busy and continue to process other jobs.



revision e58044b667bcc6c8b776d30fa04dbbbfdb49ebde
Author: dat.tq.phan <dat...@de...>
Date:   Mon, 1 Apr 2024 18:14:56 +0700

ntf: remove overdue notification from logger buffer [#3349]

When logger buffer on NTF is full, it cannot receive requests.
Sometimes, notifications stay in logger buffer for too long make
NTF always busy.

Solution is to remove overdue notification out of logger buffer
so NTF can continue receive requests.



Complete diffstat:
------------------
 src/amf/amfd/ntf.cc             |  7 +++++++
 src/ntf/ntfd/NtfLogger.cc       | 18 ++++++++++++++++++
 src/ntf/ntfd/NtfLogger.h        |  1 +
 src/ntf/ntfd/NtfNotification.cc |  9 +++++++++
 src/ntf/ntfd/NtfNotification.h  |  3 +++
 src/ntf/ntfd/ntfs.h             |  2 ++
 6 files changed, 40 insertions(+)


Testing Commands:
-----------------
*** LIST THE COMMAND LINE TOOLS/STEPS TO TEST YOUR CHANGES ***


Testing, Expected Results:
--------------------------
*** PASTE COMMAND OUTPUTS / TEST RESULTS ***


Conditions of Submission:
-------------------------
*** HOW MANY DAYS BEFORE PUSHING, CONSENSUS ETC ***


Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.

Re: [devel] [PATCH 1/1] ntf: ntfd cannot handle notification when logd overload [#3349]

[Thien M. H. <thi...@de...>]

Hi Dat,

Please check my comments.

  *   Short commit message should be describe the changeset and starting with a verb in present tense (e.g. Add, Fix, Change, ...)
  *
Should separate this ticket in 2 commits (NTF and AMF).

See my comments inline [Thien]

Best Regards,
Thien

________________________________
From: Dat Tran Quoc Phan <dat...@de...>
Sent: Friday, March 29, 2024 8:06 AM
To: Thang Duc Nguyen <tha...@de...>; Thien Minh Huynh <thi...@de...>; Tai Huynh Nguyen <tai...@de...>
Cc: ope...@li... <ope...@li...>; Dat Tran Quoc Phan <dat...@de...>
Subject: [PATCH 1/1] ntf: ntfd cannot handle notification when logd overload [#3349]

When AMF sends job to NTF but NTF is busy with full buffer,
AMF will be stuck and unable to process other jobs.
When logger buffer on NTF is full, it cannot receive requests.
Sometimes, notfications stay in logger buffer for too long make
NTF always busy.
[Thien] Correct typo notfications ->notifications

This enhancement will improve notification processing.
For AMF, it will move the front NTF job to the end of queue
when NTF is busy and continue process other jobs.
For NTF, it will remove overdue notification out of buffer
so NTF can continue receive requests.
---
 src/amf/amfd/ntf.cc             |  7 +++++++
 src/ntf/ntfd/NtfLogger.cc       | 18 ++++++++++++++++++
 src/ntf/ntfd/NtfLogger.h        |  1 +
 src/ntf/ntfd/NtfNotification.cc |  9 +++++++++
 src/ntf/ntfd/NtfNotification.h  |  4 ++++
 5 files changed, 39 insertions(+)

diff --git a/src/amf/amfd/ntf.cc b/src/amf/amfd/ntf.cc
index 52ee7456d..74fe01bae 100644
--- a/src/amf/amfd/ntf.cc
+++ b/src/amf/amfd/ntf.cc
@@ -809,6 +809,13 @@ AvdJobDequeueResultT NtfSend::exec(AVD_CL_CB* cb) {
     res = JOB_EXECUTED;
   } else if (rc == SA_AIS_ERR_TRY_AGAIN) {
     TRACE("TRY-AGAIN");
+    // In case NtfSend job and NTF service is busy with full buffer
+    // the AMFD will stuck to process this job. As consequence another
+    // job type will not be executed. Solution is to pop then put it
+    // into end of the queue.
+    Job* tmp = Fifo::dequeue();
+    TRACE("Move front to the end of queue");
+    Fifo::queue(tmp);
     res = JOB_ETRYAGAIN;
   } else if (rc == SA_AIS_ERR_TIMEOUT) {
     TRACE("TIMEOUT");
diff --git a/src/ntf/ntfd/NtfLogger.cc b/src/ntf/ntfd/NtfLogger.cc
index c0ecc4cb7..2b5c501ee 100644
--- a/src/ntf/ntfd/NtfLogger.cc
+++ b/src/ntf/ntfd/NtfLogger.cc
@@ -108,6 +108,12 @@ void saLogStreamOpenCallback(SaInvocationT invocation,

 void saLogWriteLogCallback(SaInvocationT invocation, SaAisErrorT error) {
   TRACE_ENTER2("Callback for notificationId %llu", invocation);
+  if (NtfAdmin::theNtfAdmin->logger.getFrontNotificationId() != invocation) {
+    TRACE("Notification had been processed by logd, but Id is not existed"
+          " in logger. Probably due to notification overdue, ignore "
+          "notificationId %llu", invocation);
+    return;
+  }
   if (SA_AIS_OK != error) {
     TRACE_1("Error when logging (%d)", error);
     NtfAdmin::theNtfAdmin->logger.disableAckWaiting();
@@ -378,9 +384,21 @@ void NtfLogger::disableAckWaiting() {
   notification->setWaitingAck(false);
 }

+SaNtfIdentifierT NtfLogger::getFrontNotificationId() {
+  return (queuedNotificationList.front()->getNotificationId());
+}
+
 void NtfLogger::logQueuedNotification() {
   if (!isLoggerBufferEmpty()) {
     NtfSmartPtr notification = queuedNotificationList.front();
+    if (notification->is_overdue() && notification->isWaitingAck()) {
+      LOG_NO("Notification overdue, remove notification Id: %llu",
+             notification->getNotificationId());
+      dequeueNotification();
+      resetLoggerBufferFullFlag();
+      sendLoggedConfirm(notification->getNotificationId());
+      notification = queuedNotificationList.front();
+    }
     if (notification->isWaitingAck()) return;
     TRACE_2("Log queued notification: %llu",
             notification->getNotificationId());
diff --git a/src/ntf/ntfd/NtfLogger.h b/src/ntf/ntfd/NtfLogger.h
index c7cf644e6..283fd3fe2 100644
--- a/src/ntf/ntfd/NtfLogger.h
+++ b/src/ntf/ntfd/NtfLogger.h
@@ -73,6 +73,7 @@ class NtfLogger {
   void logQueuedNotification();
   bool isLoggerBufferEmpty() { return queuedNotificationList.empty(); }
   void disableAckWaiting();
+  SaNtfIdentifierT getFrontNotificationId();

  private:
   SaAisErrorT initLog();
diff --git a/src/ntf/ntfd/NtfNotification.cc b/src/ntf/ntfd/NtfNotification.cc
index b23d0dde8..0380c5d49 100644
--- a/src/ntf/ntfd/NtfNotification.cc
+++ b/src/ntf/ntfd/NtfNotification.cc
@@ -49,6 +49,7 @@ NtfNotification::NtfNotification(SaNtfIdentifierT notificationId,
   ntfsv_get_ntf_header(sendNotInfo_, &header);
   *header->notificationId = notificationId_;
   notificationType_ = notificationType; /* deleted in destructor */
+  queue_at_ = base::TimespecToNanos(base::ReadMonotonicClock());
 }

 /**
@@ -266,6 +267,14 @@ void NtfNotification::removeSubscription(unsigned int clientId,
  */
 ntfsv_send_not_req_t* NtfNotification::getNotInfo() { return sendNotInfo_; }

+bool NtfNotification::is_overdue() const {
+  uint32_t max_time = NTFSV_NOTIFICATION_MAX_QUEUE_TIME_DEFFAULT;
+  timespec queue_at = base::NanosToTimespec(queue_at_);
+  timespec current = base::ReadMonotonicClock();
+  timespec max_queue_time{static_cast<time_t>(max_time), 0};
+  return (current - queue_at > max_queue_time);
+}
+
 /**
  * This method is called if a newly started standby asks for
  * synchronization.
diff --git a/src/ntf/ntfd/NtfNotification.h b/src/ntf/ntfd/NtfNotification.h
index 4de8e14c6..a0515908c 100644
--- a/src/ntf/ntfd/NtfNotification.h
+++ b/src/ntf/ntfd/NtfNotification.h
@@ -29,8 +29,10 @@
 #include "ntfs_com.h"
 #include "ntf/common/ntfsv_msg.h"
 #include <tr1/memory>
+#include "base/time.h"

 #define NACK_THRESHOLD 4
+#define NTFSV_NOTIFICATION_MAX_QUEUE_TIME_DEFFAULT 10
[Thien]: Naming should be closest to the purpose
I suggest renaming to
#define NTFSV_LOGGER_RECORD_TIMEOUT 10
and define in file ntfs.h instead of this file

 typedef struct {
   unsigned int clientId;
@@ -67,6 +69,7 @@ class NtfNotification {
   SaNtfNotificationHeaderT* header();
   ntfsv_send_not_req_t* sendNotInfo_;
   bool loggFromCallback_;
+  bool is_overdue() const;

  private:
   NtfNotification();
@@ -80,6 +83,7 @@ class NtfNotification {
   typedef std::list<UniqueSubscriptionId> SubscriptionList;
   SubscriptionList subscriptionList;
   SubscriptionList::iterator idListPos;
+  uint64_t queue_at_;  // record time when notf enter queue
 };

 typedef std::tr1::shared_ptr<NtfNotification> NtfSmartPtr;
--
2.25.1

Re: [devel] [PATCH 1/1] ntf: ntfd cannot handle notification when logd overload [#3349]

[Thien M. H. <thi...@de...>]

Hi Dat,

See my comments inline [Thien].

Best Regards,
Thien

________________________________
From: Dat Tran Quoc Phan <dat...@de...>
Sent: Monday, March 25, 2024 8:52 AM
To: Thang Duc Nguyen <tha...@de...>; Thien Minh Huynh <thi...@de...>
Cc: ope...@li... <ope...@li...>; Dat Tran Quoc Phan <dat...@de...>
Subject: [PATCH 1/1] ntf: ntfd cannot handle notification when logd overload [#3349]

When AMF sends job to NTF but NTF is busy with full buffer,
AMF will be stuck and unable to process other jobs.
When logger buffer on NTF is full, it cannot receive requests.
Sometimes, notfications stay in logger buffer for too long make
NTF always busy.

This enhancement will improve notification processing.
For AMF, it will move the front NTF job to the end of queue
when NTF is busy and continue process other jobs.
For NTF, it will remove overdue notification out of buffer
so NTF can continue receive requests.
---
 src/amf/amfd/ntf.cc             | 11 +++++++++++
 src/ntf/ntfd/NtfLogger.cc       | 19 +++++++++++++++++++
 src/ntf/ntfd/NtfLogger.h        |  1 +
 src/ntf/ntfd/NtfNotification.cc | 13 +++++++++++++
 src/ntf/ntfd/NtfNotification.h  |  5 +++++
 5 files changed, 49 insertions(+)

diff --git a/src/amf/amfd/ntf.cc b/src/amf/amfd/ntf.cc
index 52ee7456d..96cff94b3 100644
--- a/src/amf/amfd/ntf.cc
+++ b/src/amf/amfd/ntf.cc
@@ -809,6 +809,17 @@ AvdJobDequeueResultT NtfSend::exec(AVD_CL_CB* cb) {
     res = JOB_EXECUTED;
   } else if (rc == SA_AIS_ERR_TRY_AGAIN) {
     TRACE("TRY-AGAIN");
+    // In case NtfSend job and NTF service is busy with full buffer
+    // the AMFD will stuck to process this job. As consequence another
+    // job type will not be executed. Solution is to pop then put it
+    // into end of the queue.
+    Job* tmp = Fifo::dequeue();
+    if (tmp) {
+      TRACE("Move front to end of queue");
+      Fifo::queue(tmp);
+    } else {
+      TRACE("Job unavailable");
+    }
     res = JOB_ETRYAGAIN;
   } else if (rc == SA_AIS_ERR_TIMEOUT) {
     TRACE("TIMEOUT");
diff --git a/src/ntf/ntfd/NtfLogger.cc b/src/ntf/ntfd/NtfLogger.cc
index c0ecc4cb7..ad4459156 100644
--- a/src/ntf/ntfd/NtfLogger.cc
+++ b/src/ntf/ntfd/NtfLogger.cc
@@ -108,6 +108,12 @@ void saLogStreamOpenCallback(SaInvocationT invocation,

 void saLogWriteLogCallback(SaInvocationT invocation, SaAisErrorT error) {
   TRACE_ENTER2("Callback for notificationId %llu", invocation);
+  if (!NtfAdmin::theNtfAdmin->logger.isNotificationInQueue(invocation)) {
+    // notification processed on LogS side, there is a chance
+    // this notification had been dequeued due to overdue
+    // cignore this allback to avoid dequeue wrong notification
+    return;
+  }
   if (SA_AIS_OK != error) {
     TRACE_1("Error when logging (%d)", error);
     NtfAdmin::theNtfAdmin->logger.disableAckWaiting();
@@ -378,9 +384,22 @@ void NtfLogger::disableAckWaiting() {
   notification->setWaitingAck(false);
 }

+bool NtfLogger::isNotificationInQueue(SaInvocationT notfId) {
+  // dequeueNotification pop the first element so check the first one only
+  return (queuedNotificationList.front()->getNotificationId() == notfId);
+}
+
 void NtfLogger::logQueuedNotification() {
   if (!isLoggerBufferEmpty()) {
     NtfSmartPtr notification = queuedNotificationList.front();
+    if (notification->is_overdue() && notification->isWaitingAck()) {
+      LOG_NO("Notification overdue, remove notification Id: %llu",
+             notification->getNotificationId());
+      dequeueNotification();
+      resetLoggerBufferFullFlag();
+      sendLoggedConfirm(notification->getNotificationId());
+      return;

[Thien] if a notification is overdue, I think should handle next one instead of wait until next period.
Because the next notification message has not written yet.
e.g.
NtfSmartPtr notification = queuedNotificationList.front();
// do not return
+    }
     if (notification->isWaitingAck()) return;
     TRACE_2("Log queued notification: %llu",
             notification->getNotificationId());
diff --git a/src/ntf/ntfd/NtfLogger.h b/src/ntf/ntfd/NtfLogger.h
index c7cf644e6..eb9dd7b76 100644
--- a/src/ntf/ntfd/NtfLogger.h
+++ b/src/ntf/ntfd/NtfLogger.h
@@ -73,6 +73,7 @@ class NtfLogger {
   void logQueuedNotification();
   bool isLoggerBufferEmpty() { return queuedNotificationList.empty(); }
   void disableAckWaiting();
+  bool isNotificationInQueue(SaInvocationT);

  private:
   SaAisErrorT initLog();
diff --git a/src/ntf/ntfd/NtfNotification.cc b/src/ntf/ntfd/NtfNotification.cc
index b23d0dde8..abef4d447 100644
--- a/src/ntf/ntfd/NtfNotification.cc
+++ b/src/ntf/ntfd/NtfNotification.cc
@@ -49,6 +49,7 @@ NtfNotification::NtfNotification(SaNtfIdentifierT notificationId,
   ntfsv_get_ntf_header(sendNotInfo_, &header);
   *header->notificationId = notificationId_;
   notificationType_ = notificationType; /* deleted in destructor */
+  queue_at_ = base::TimespecToNanos(base::ReadMonotonicClock());
 }

 /**
@@ -266,6 +267,18 @@ void NtfNotification::removeSubscription(unsigned int clientId,
  */
 ntfsv_send_not_req_t* NtfNotification::getNotInfo() { return sendNotInfo_; }

+uint32_t NtfNotification::getMaxQueueTime() const {
+  return NTFSV_NOTIFICATION_MAX_QUEUE_TIME_DEFFAULT;
[Thien]: This function is not handle any logic. That just return a constant. I think use constant directly will be better than instead of create new function.
+}
+
+bool NtfNotification::is_overdue() const {
+  uint32_t max_time = getMaxQueueTime();
+  timespec queue_at = base::NanosToTimespec(queue_at_);
+  timespec current = base::ReadMonotonicClock();
+  timespec max_queue_time{static_cast<time_t>(max_time), 0};
+  return (current - queue_at > max_queue_time);
+}
+
 /**
  * This method is called if a newly started standby asks for
  * synchronization.
diff --git a/src/ntf/ntfd/NtfNotification.h b/src/ntf/ntfd/NtfNotification.h
index 4de8e14c6..9e56b8d8a 100644
--- a/src/ntf/ntfd/NtfNotification.h
+++ b/src/ntf/ntfd/NtfNotification.h
@@ -29,8 +29,10 @@
 #include "ntfs_com.h"
 #include "ntf/common/ntfsv_msg.h"
 #include <tr1/memory>
+#include "base/time.h"

 #define NACK_THRESHOLD 4
+#define NTFSV_NOTIFICATION_MAX_QUEUE_TIME_DEFFAULT 10

 typedef struct {
   unsigned int clientId;
@@ -67,6 +69,8 @@ class NtfNotification {
   SaNtfNotificationHeaderT* header();
   ntfsv_send_not_req_t* sendNotInfo_;
   bool loggFromCallback_;
+  uint32_t getMaxQueueTime() const;
+  bool is_overdue() const;

  private:
   NtfNotification();
@@ -80,6 +84,7 @@ class NtfNotification {
   typedef std::list<UniqueSubscriptionId> SubscriptionList;
   SubscriptionList subscriptionList;
   SubscriptionList::iterator idListPos;
+  uint64_t queue_at_;  // record time when notf enter queue
 };

 typedef std::tr1::shared_ptr<NtfNotification> NtfSmartPtr;
--
2.25.1

Re: [devel] [PATCH 1/1] smf: Fix errors reported by valgrind [#3347]

[Thien M. H. <thi...@de...>]

Hi Tai,

Please check my comments [Thien].

Best Regards,
Thien

________________________________
From: Tai Huynh Nguyen <tai...@de...>
Sent: Wednesday, February 28, 2024 6:02 PM
To: Dat Tran Quoc Phan <dat...@de...>; Thang Duc Nguyen <tha...@de...>; Thien Minh Huynh <thi...@de...>
Cc: ope...@li... <ope...@li...>; Tai Huynh Nguyen <tai...@de...>
Subject: [PATCH 1/1] smf: Fix errors reported by valgrind [#3347]

Fix access uninitialised value, definitely lost and mismatched free()
---
 src/smf/common/smfsv_evt.c        | 12 ++++++------
 src/smf/smfd/SmfCampaignThread.cc | 11 ++++++++++-
 src/smf/smfd/SmfExecControl.cc    |  3 ++-
 src/smf/smfd/SmfUpgradeStep.cc    |  2 +-
 src/smf/smfd/SmfUtils_ObjExist.cc |  2 +-
 src/smf/smfd/smfd_campaign_oi.cc  | 10 +++++++++-
 6 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/src/smf/common/smfsv_evt.c b/src/smf/common/smfsv_evt.c
index 2df889fca..d28621401 100644
--- a/src/smf/common/smfsv_evt.c
+++ b/src/smf/common/smfsv_evt.c
@@ -168,15 +168,15 @@ uint32_t smf_enc_cbk_rsp(SMF_RESP_EVT *i_evt, NCS_UBAID *o_ub)
         uint32_t rc = NCSCC_RC_SUCCESS;
         uint8_t *p8;

-       p8 = ncs_enc_reserve_space(o_ub, sizeof(SMF_RESP_EVT));
+       p8 = ncs_enc_reserve_space(o_ub, 8);
[Thien]: Should not decrease this size , It's NBC. Coredump will happens when upgrade.
should keep and encoding more 4byte

         if (p8 == NULL) {
                 LOG_ER("ncs_enc_reserve_space failed");
                 goto err;
         }
-       ncs_encode_64bit(&p8, i_evt->inv_id);
+       ncs_encode_32bit(&p8, i_evt->inv_id);
         ncs_encode_32bit(&p8, i_evt->err);

-       ncs_enc_claim_space(o_ub, sizeof(SMF_RESP_EVT));
+       ncs_enc_claim_space(o_ub, 8);

         return rc;
 err:
@@ -229,9 +229,9 @@ uint32_t smf_dec_cbk_rsp(NCS_UBAID *i_ub, SMF_RESP_EVT *o_evt)
                 goto err;
         }

-       p8 = ncs_dec_flatten_space(i_ub, local_data, 8);
-       o_evt->inv_id = ncs_decode_64bit(&p8);
-       ncs_dec_skip_space(i_ub, 8);
+       p8 = ncs_dec_flatten_space(i_ub, local_data, 4);
+       o_evt->inv_id = ncs_decode_32bit(&p8);
+       ncs_dec_skip_space(i_ub, 4);

         p8 = ncs_dec_flatten_space(i_ub, local_data, 4);
         o_evt->err = ncs_decode_32bit(&p8);
diff --git a/src/smf/smfd/SmfCampaignThread.cc b/src/smf/smfd/SmfCampaignThread.cc
index 1f76c3e5a..ba56844ca 100644
--- a/src/smf/smfd/SmfCampaignThread.cc
+++ b/src/smf/smfd/SmfCampaignThread.cc
@@ -493,7 +493,11 @@ void SmfCampaignThread::ntfNotificationCallback(
             notification->notificationType);
     }
   } while (false);
-
+  if (notification->notificationType == SA_NTF_TYPE_STATE_CHANGE) {
+      saNtfNotificationFree(
+      notification->notification.stateChangeNotification
+      .notificationHandle);
+  }
   TRACE_LEAVE();
 }
 /**
@@ -669,6 +673,9 @@ int SmfCampaignThread::sendStateNotification(
     return -1;
   }

+  memset(ntfStateNot.notificationHeader.correlatedNotifications, 0,
+        ntfStateNot.notificationHeader.numCorrelatedNotifications*
+        sizeof(SaNtfIdentifierT));
   /* Notifying object */
   size_t length = sizeof(SMF_NOTIFYING_OBJECT);
   if (length > static_cast<size_t>(GetSmfMaxDnLength())) {
@@ -682,6 +689,7 @@ int SmfCampaignThread::sendStateNotification(
     result = -1;
     goto done;
   }
+  memset(value, '\0', length + 1);
[Thien]: should use length instead length + 1. Because the last position has been filled by line "value[length] = '\0';"

   memcpy(value, SMF_NOTIFYING_OBJECT, length - 1);
   value[length] = '\0';
   osaf_extended_name_steal(value,
@@ -700,6 +708,7 @@ int SmfCampaignThread::sendStateNotification(
     result = -1;
     goto done;
   }
+  memset(value, '\0', length + 1);
[Thien]: should use length instead length + 1. Because the last position has been filled by line "value[length] = '\0';"

   memcpy(value, dn.c_str(), length - 1);
   value[length] = '\0';
   osaf_extended_name_steal(value,
diff --git a/src/smf/smfd/SmfExecControl.cc b/src/smf/smfd/SmfExecControl.cc
index 27584385f..87ebd4a1b 100644
--- a/src/smf/smfd/SmfExecControl.cc
+++ b/src/smf/smfd/SmfExecControl.cc
@@ -89,7 +89,7 @@ bool createBalancedProcs() {
     ssproc->setProcName("safSmfProc=SmfBalancedProc" + std::to_string(procnum));
     // Balanced procedures may not be run in parallel
     ssproc->setExecLevel(std::to_string(procnum));
-    ssproc->setIsMergedProcedure(true);  // For cleanup purposes
+    ssproc->setIsMergedProcedure(false);  // For cleanup purposes
     // Each new procedure holds the balanced group it steps over
     ssproc->setBalancedGroup(std::vector<std::string>(itr, iend));
     balancedprocs.push_back(ssproc);
@@ -337,6 +337,7 @@ SmfUpgradeStep* mergeStep(SmfUpgradeProcedure* procedure,
       TRACE("adding modifications and bundle ref from node:%s",
             step->getSwNode().c_str());
       newstep->addModifications(step->getModifications());
+      step->getModifications().clear();
       procedure->mergeBundleRefRollingToSingleStep(newstep, step);
     }
   }
diff --git a/src/smf/smfd/SmfUpgradeStep.cc b/src/smf/smfd/SmfUpgradeStep.cc
index e012ef913..6ba5a2c66 100644
--- a/src/smf/smfd/SmfUpgradeStep.cc
+++ b/src/smf/smfd/SmfUpgradeStep.cc
@@ -2180,7 +2180,7 @@ bool SmfUpgradeStep::waitForBundleCmdResult(
             }

             i_swNodeList.remove(evt->event.swResult.nodeName);
-            delete (evt->event.swResult.nodeName);
+            free(evt->event.swResult.nodeName);
             delete (evt);
             break;
           }
diff --git a/src/smf/smfd/SmfUtils_ObjExist.cc b/src/smf/smfd/SmfUtils_ObjExist.cc
index f89317a08..5278b870c 100644
--- a/src/smf/smfd/SmfUtils_ObjExist.cc
+++ b/src/smf/smfd/SmfUtils_ObjExist.cc
@@ -291,6 +291,6 @@
       // Failed. No valid object RDN is found
       object_rdn_.clear();
     }
-
+    saImmOmClassDescriptionMemoryFree_2(om_handle_, attr_definitions);
     return rc;
   }
diff --git a/src/smf/smfd/smfd_campaign_oi.cc b/src/smf/smfd/smfd_campaign_oi.cc
index 831b300f8..ffd6063d6 100644
--- a/src/smf/smfd/smfd_campaign_oi.cc
+++ b/src/smf/smfd/smfd_campaign_oi.cc
@@ -1210,22 +1210,30 @@ uint32_t read_config_and_set_control_block(smfd_cb_t *cb) {
   } else {
     LOG_NO("smfKeepDuState = %d", *keepDuState);
   }
-
+  if (cb->backupCreateCmd != NULL) free(cb->backupCreateCmd);
   cb->backupCreateCmd = strdup(backupCreateCmd);
+  if (cb->bundleCheckCmd != NULL) free(cb->bundleCheckCmd);
   cb->bundleCheckCmd = strdup(bundleCheckCmd);
+  if (cb->nodeCheckCmd != NULL) free(cb->nodeCheckCmd);
   cb->nodeCheckCmd = strdup(nodeCheckCmd);
+  if (cb->repositoryCheckCmd != NULL) free(cb->repositoryCheckCmd);
   cb->repositoryCheckCmd = strdup(repositoryCheckCmd);
+  if (cb->clusterRebootCmd != NULL) free(cb->clusterRebootCmd);
   cb->clusterRebootCmd = strdup(clusterRebootCmd);
   cb->adminOpTimeout = *adminOpTimeout;
   cb->cliTimeout = *cliTimeout;
   cb->rebootTimeout = *rebootTimeout;
   if ((smfNodeBundleActCmd != NULL) && (strcmp(smfNodeBundleActCmd, "") != 0)) {
+    if (cb->nodeBundleActCmd != NULL) free(cb->nodeBundleActCmd);
     cb->nodeBundleActCmd = strdup(smfNodeBundleActCmd);
   }
+  if (cb->smfSiSwapSiName != NULL) free (cb->smfSiSwapSiName);
   cb->smfSiSwapSiName = strdup(smfSiSwapSiName);
   cb->smfSiSwapMaxRetry = *smfSiSwapMaxRetry;
   cb->smfCampMaxRestart = *smfCampMaxRestart;
+  if (cb->smfImmPersistCmd != NULL) free(cb->smfImmPersistCmd);
   cb->smfImmPersistCmd = strdup(smfImmPersistCmd);
+  if (cb->smfNodeRebootCmd != NULL) free(cb->smfNodeRebootCmd);
   cb->smfNodeRebootCmd = strdup(smfNodeRebootCmd);
   cb->smfInactivatePbeDuringUpgrade = *smfInactivatePbeDuringUpgrade;
   cb->smfVerifyEnable = *smfVerifyEnable;
--
2.25.1

[devel] OpenSAF 5.24.02 release

[Gary L. <gar...@de...>]

The OpenSAF community is pleased to announce the availability of the OpenSAF 5.24.02 release.

The source code for OpenSAF 5.24.02 and the corresponding documentation can be downloaded using the following links: [opensaf-5.24.02.tar.gz](http://sourceforge.net/projects/opensaf/files/releases/opensaf-5.24.02.tar.gz/download), [opensaf-documentation-5.24.02.tar.gz](http://sourceforge.net/projects/opensaf/files/docs/opensaf-documentation-5.24.02.tar.gz/download).

For a complete list of new features in this release, please refer to the [NEWS](https://sourceforge.net/p/opensaf/wiki/NEWS-5.24.02/) at the wiki.

See the [ChangeLog](https://sourceforge.net/p/opensaf/wiki/ChangeLog-5.24.02/) for a full list of changes in this release.

Re: [devel] [PATCH 1/1] osaf: supports compile/build with gcc/g++ 12 [#3346]

[Thien M. H. <thi...@de...>]

Hi Thang,

ACK from me.

Best Regards,
Thien

________________________________
From: Thang Duc Nguyen <tha...@de...>
Sent: Monday, January 15, 2024 5:42 PM
To: Thien Minh Huynh <thi...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li... <ope...@li...>; Thang Duc Nguyen <tha...@de...>
Subject: [PATCH 1/1] osaf: supports compile/build with gcc/g++ 12 [#3346]

Fix error when compiling/building Opensaf using
gcc/g++ 12.
---
 src/amf/amfd/node.h                |  5 +++-
 src/dtm/dtmnd/dtm_node_db.cc       |  2 +-
 src/imm/immnd/immnd_proc.c         |  2 +-
 src/osaf/consensus/key_value.cc    |  1 +
 src/smf/smfd/SmfProcedureThread.cc | 43 +++++++++++++++---------------
 5 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/src/amf/amfd/node.h b/src/amf/amfd/node.h
index 37b8ce938..3456adb85 100644
--- a/src/amf/amfd/node.h
+++ b/src/amf/amfd/node.h
@@ -166,7 +166,10 @@ class AVD_AVND {
 bool operator<(const AVD_AVND &lhs, const AVD_AVND &rhs);

 struct NodeNameCompare
-    : public std::binary_function<AVD_AVND *, AVD_AVND *, bool> {
+{
+  typedef AVD_AVND first_argument_type;
+  typedef AVD_AVND second_argument_type;
+  typedef bool result_type;
   bool operator()(const AVD_AVND *lhs, const AVD_AVND *rhs);
 };

diff --git a/src/dtm/dtmnd/dtm_node_db.cc b/src/dtm/dtmnd/dtm_node_db.cc
index 679368971..aa7125b68 100644
--- a/src/dtm/dtmnd/dtm_node_db.cc
+++ b/src/dtm/dtmnd/dtm_node_db.cc
@@ -292,7 +292,7 @@ uint32_t dtm_node_delete(DTM_NODE_DB *node, KeyTypes type) {
       break;

     case KeyTypes::kDtmNodeIpKeyType:
-      if (node->node_ip != nullptr && node->pat_ip_address.key_info) {
+      if (strlen(node->node_ip) != 0 && node->pat_ip_address.key_info) {
         TRACE("DTM:Deleting node_ip from the  database with node_ip :%s as key",
               node->node_ip);
         if ((rc = ncs_patricia_tree_del(&dtms_cb->ip_addr_tree,
diff --git a/src/imm/immnd/immnd_proc.c b/src/imm/immnd/immnd_proc.c
index bf14d18a8..5df703799 100644
--- a/src/imm/immnd/immnd_proc.c
+++ b/src/imm/immnd/immnd_proc.c
@@ -1901,7 +1901,7 @@ static int immnd_forkPbe(IMMND_CB *cb)
                 return -1;
         }

-       strncpy(execPath, cb->mProgName, execDirLen);
+       memcpy(execPath, cb->mProgName, execDirLen);
         execPath[execDirLen] = 0;
         if ((execDirLen == 0) || (cb->mProgName[execDirLen - 1] != '/'))
                 strncat(execPath, "/", 2);
diff --git a/src/osaf/consensus/key_value.cc b/src/osaf/consensus/key_value.cc
index 692dd3f1d..b3417c735 100644
--- a/src/osaf/consensus/key_value.cc
+++ b/src/osaf/consensus/key_value.cc
@@ -18,6 +18,7 @@
 #include "base/getenv.h"
 #include "base/logtrace.h"
 #include "osaf/consensus/consensus.h"
+#include <array>

 int KeyValue::Execute(const std::string& command, std::string& output) {
   TRACE_ENTER();
diff --git a/src/smf/smfd/SmfProcedureThread.cc b/src/smf/smfd/SmfProcedureThread.cc
index f41e89288..db52895dc 100644
--- a/src/smf/smfd/SmfProcedureThread.cc
+++ b/src/smf/smfd/SmfProcedureThread.cc
@@ -441,30 +441,31 @@ SaAisErrorT SmfProcedureThread::getImmProcedure(
   implementorName = immutil_getStringAttr(
       (const SaImmAttrValuesT_2 **)attributes, SA_IMM_ATTR_IMPLEMENTER_NAME, 0);

-  if ((implementorName != NULL) &&
-      (strcmp(implementorName, procedure->getProcName().c_str())) &&
+  if (implementorName != NULL) {
+    if(strcmp(implementorName, procedure->getProcName().c_str()) &&
       strncmp(implementorName, SMF_PROC_OI_NAME_PREFIX,
               strlen(SMF_PROC_OI_NAME_PREFIX))) {
-    /* The implementor name:
-     *    -is not the procedure name (newer implementation of SMF)
-     *    -and does not start with the SMF procedure OI name prefix (even newer
-     * implementation of SMF) which means the procedure object was created by an
-     * old version of SMF. So we have to continue using this old implementor
-     * name (i.e. IMM handle) for this procedure. This is just to be able to
-     * handle the upgrade case where a new opensaf is upgraded by an old opensaf
-     * version (which used the campaign Dn as implementor name for everything).
-     */
-    LOG_NO("SmfProcedureThread::getImmProcedure, Using campaign IMM handle %s",
-           implementorName);
-    m_useCampaignOiHandle = true;
-  } else {
-    LOG_NO("SmfProcedureThread::getImmProcedure, Using own IMM handle %s",
-           implementorName);
-    // Overwrite the already existing OI name (that was generated by the
-    // constructor)
-    procedure->setProcOiName(implementorName);
+      /* The implementor name:
+      *    -is not the procedure name (newer implementation of SMF)
+      *    -and does not start with the SMF procedure OI name prefix (even newer
+      * implementation of SMF) which means the procedure object was created by
+      * an old version of SMF. So we have to continue using this old
+      * implementor name (i.e. IMM handle) for this procedure. This is just to
+      * be able to handle the upgrade case where a new opensaf is upgraded by
+      * an old opensaf version (which used the campaign Dn as implementor name
+      * for everything).
+      */
+      LOG_NO("SmfProcedureThread::getImmProcedure, Using campaign IMM \
+            handle %s", implementorName);
+      m_useCampaignOiHandle = true;
+    } else {
+      LOG_NO("SmfProcedureThread::getImmProcedure, Using own IMM handle %s",
+            implementorName);
+      // Overwrite the already existing OI name (that was generated by the
+      // constructor)
+      procedure->setProcOiName(implementorName);
+    }
   }
-
 done:
   TRACE_LEAVE();
   return rc;
--
2.25.1

[devel] [PATCH 0/1] Review Request for osaf: supports compile/build with gcc/g++ 12 [#3346]

[thang.d.nguyen <tha...@de...>]

Summary: osaf: supports compile/build with gcc/g++ 12 [#3346]
Review request for Ticket(s): 3346
Peer Reviewer(s): Thien, Dat
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3346
Base revision: f8fd609eb95fa347b0f1f94cb73bbd94fac61229
Personal repository: git://git.code.sf.net/u/thangng/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            y
 OpenSAF services        n
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   n


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

revision e01bbd5f649452acf4df8046fc8fcacd30bac792
Author:	thang.d.nguyen <tha...@de...>
Date:	Mon, 15 Jan 2024 17:23:45 +0700

osaf: supports compile/build with gcc/g++ 12 [#3346]

Fix error when compiling/building Opensaf using
gcc/g++ 12.



Complete diffstat:
------------------
 src/amf/amfd/node.h                |  5 ++++-
 src/dtm/dtmnd/dtm_node_db.cc       |  2 +-
 src/imm/immnd/immnd_proc.c         |  2 +-
 src/osaf/consensus/key_value.cc    |  1 +
 src/smf/smfd/SmfProcedureThread.cc | 43 +++++++++++++++++++-------------------
 5 files changed, 29 insertions(+), 24 deletions(-)


Testing Commands:
-----------------
N/A

Testing, Expected Results:
--------------------------
N/A

Conditions of Submission:
-------------------------
Ack from reviewer

Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.

[devel] [PATCH 1/1] osaf: supports compile/build with gcc/g++ 12 [#3346]

[thang.d.nguyen <tha...@de...>]

Fix error when compiling/building Opensaf using
gcc/g++ 12.
---
 src/amf/amfd/node.h                |  5 +++-
 src/dtm/dtmnd/dtm_node_db.cc       |  2 +-
 src/imm/immnd/immnd_proc.c         |  2 +-
 src/osaf/consensus/key_value.cc    |  1 +
 src/smf/smfd/SmfProcedureThread.cc | 43 +++++++++++++++---------------
 5 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/src/amf/amfd/node.h b/src/amf/amfd/node.h
index 37b8ce938..3456adb85 100644
--- a/src/amf/amfd/node.h
+++ b/src/amf/amfd/node.h
@@ -166,7 +166,10 @@ class AVD_AVND {
 bool operator<(const AVD_AVND &lhs, const AVD_AVND &rhs);
 
 struct NodeNameCompare
-    : public std::binary_function<AVD_AVND *, AVD_AVND *, bool> {
+{
+  typedef AVD_AVND first_argument_type;
+  typedef AVD_AVND second_argument_type;
+  typedef bool result_type;
   bool operator()(const AVD_AVND *lhs, const AVD_AVND *rhs);
 };
 
diff --git a/src/dtm/dtmnd/dtm_node_db.cc b/src/dtm/dtmnd/dtm_node_db.cc
index 679368971..aa7125b68 100644
--- a/src/dtm/dtmnd/dtm_node_db.cc
+++ b/src/dtm/dtmnd/dtm_node_db.cc
@@ -292,7 +292,7 @@ uint32_t dtm_node_delete(DTM_NODE_DB *node, KeyTypes type) {
       break;
 
     case KeyTypes::kDtmNodeIpKeyType:
-      if (node->node_ip != nullptr && node->pat_ip_address.key_info) {
+      if (strlen(node->node_ip) != 0 && node->pat_ip_address.key_info) {
         TRACE("DTM:Deleting node_ip from the  database with node_ip :%s as key",
               node->node_ip);
         if ((rc = ncs_patricia_tree_del(&dtms_cb->ip_addr_tree,
diff --git a/src/imm/immnd/immnd_proc.c b/src/imm/immnd/immnd_proc.c
index bf14d18a8..5df703799 100644
--- a/src/imm/immnd/immnd_proc.c
+++ b/src/imm/immnd/immnd_proc.c
@@ -1901,7 +1901,7 @@ static int immnd_forkPbe(IMMND_CB *cb)
 		return -1;
 	}
 
-	strncpy(execPath, cb->mProgName, execDirLen);
+	memcpy(execPath, cb->mProgName, execDirLen);
 	execPath[execDirLen] = 0;
 	if ((execDirLen == 0) || (cb->mProgName[execDirLen - 1] != '/'))
 		strncat(execPath, "/", 2);
diff --git a/src/osaf/consensus/key_value.cc b/src/osaf/consensus/key_value.cc
index 692dd3f1d..b3417c735 100644
--- a/src/osaf/consensus/key_value.cc
+++ b/src/osaf/consensus/key_value.cc
@@ -18,6 +18,7 @@
 #include "base/getenv.h"
 #include "base/logtrace.h"
 #include "osaf/consensus/consensus.h"
+#include <array>
 
 int KeyValue::Execute(const std::string& command, std::string& output) {
   TRACE_ENTER();
diff --git a/src/smf/smfd/SmfProcedureThread.cc b/src/smf/smfd/SmfProcedureThread.cc
index f41e89288..db52895dc 100644
--- a/src/smf/smfd/SmfProcedureThread.cc
+++ b/src/smf/smfd/SmfProcedureThread.cc
@@ -441,30 +441,31 @@ SaAisErrorT SmfProcedureThread::getImmProcedure(
   implementorName = immutil_getStringAttr(
       (const SaImmAttrValuesT_2 **)attributes, SA_IMM_ATTR_IMPLEMENTER_NAME, 0);
 
-  if ((implementorName != NULL) &&
-      (strcmp(implementorName, procedure->getProcName().c_str())) &&
+  if (implementorName != NULL) {
+    if(strcmp(implementorName, procedure->getProcName().c_str()) &&
       strncmp(implementorName, SMF_PROC_OI_NAME_PREFIX,
               strlen(SMF_PROC_OI_NAME_PREFIX))) {
-    /* The implementor name:
-     *    -is not the procedure name (newer implementation of SMF)
-     *    -and does not start with the SMF procedure OI name prefix (even newer
-     * implementation of SMF) which means the procedure object was created by an
-     * old version of SMF. So we have to continue using this old implementor
-     * name (i.e. IMM handle) for this procedure. This is just to be able to
-     * handle the upgrade case where a new opensaf is upgraded by an old opensaf
-     * version (which used the campaign Dn as implementor name for everything).
-     */
-    LOG_NO("SmfProcedureThread::getImmProcedure, Using campaign IMM handle %s",
-           implementorName);
-    m_useCampaignOiHandle = true;
-  } else {
-    LOG_NO("SmfProcedureThread::getImmProcedure, Using own IMM handle %s",
-           implementorName);
-    // Overwrite the already existing OI name (that was generated by the
-    // constructor)
-    procedure->setProcOiName(implementorName);
+      /* The implementor name:
+      *    -is not the procedure name (newer implementation of SMF)
+      *    -and does not start with the SMF procedure OI name prefix (even newer
+      * implementation of SMF) which means the procedure object was created by
+      * an old version of SMF. So we have to continue using this old
+      * implementor name (i.e. IMM handle) for this procedure. This is just to
+      * be able to handle the upgrade case where a new opensaf is upgraded by
+      * an old opensaf version (which used the campaign Dn as implementor name
+      * for everything).
+      */
+      LOG_NO("SmfProcedureThread::getImmProcedure, Using campaign IMM \
+            handle %s", implementorName);
+      m_useCampaignOiHandle = true;
+    } else {
+      LOG_NO("SmfProcedureThread::getImmProcedure, Using own IMM handle %s",
+            implementorName);
+      // Overwrite the already existing OI name (that was generated by the
+      // constructor)
+      procedure->setProcOiName(implementorName);
+    }
   }
-
 done:
   TRACE_LEAVE();
   return rc;
-- 
2.25.1

Re: [devel] [PATCH 1/1] amf: retry if imm unavailable during recovery [#3344]

[Thang D. N. <tha...@de...>]

Ack.

-----Original Message-----
From: Thien Minh Huynh <thi...@de...> 
Sent: Wednesday, January 10, 2024 10:19 AM
To: Thang Duc Nguyen <tha...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li...; Thien Minh Huynh <thi...@de...>
Subject: [PATCH 1/1] amf: retry if imm unavailable during recovery [#3344]

If a component has changed, AMF will obtain configuration from IMM when recovering it. If IMM is unavailable and SC absence is enabled, AMF will immediately fail.

Retrying the cleanup event is the solution.
This is to prevent watchdog kill AMFND.
---
 src/amf/amfnd/clc.cc    | 14 +++++++++++---
 src/amf/amfnd/compdb.cc |  6 +++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/amf/amfnd/clc.cc b/src/amf/amfnd/clc.cc index 0b0d62634..3c35a916f 100644
--- a/src/amf/amfnd/clc.cc
+++ b/src/amf/amfnd/clc.cc
@@ -1969,9 +1969,17 @@ uint32_t avnd_comp_clc_xxxing_cleansucc_hdler(AVND_CB *cb, AVND_COMP *comp) {
       "'%s': Cleanup success event in the instantiating/restarting state",
       comp->name.c_str());
   /* Refresh the component configuration, it may have changed */
-  if (!m_AVND_IS_SHUTTING_DOWN(cb) && (avnd_comp_config_reinit(comp) != 0)) {
-    rc = NCSCC_RC_FAILURE;
-    goto done;
+  if (!m_AVND_IS_SHUTTING_DOWN(cb)) {
+    int res = avnd_comp_config_reinit(comp);
+    if (res != 0) {
+      if ((res == SA_AIS_ERR_TRY_AGAIN || res == SA_AIS_ERR_TIMEOUT) &&
+          avnd_comp_clc_cmd_execute(
+              cb, comp, AVND_COMP_CLC_CMD_TYPE_CLEANUP) != NCSCC_RC_SUCCESS) {
+        TRACE("AVND_COMP_CLC_CMD_TYPE_CLEANUP retry failed");
+      }
+      rc = NCSCC_RC_FAILURE;
+      goto done;
+    }
   }
 
   /*
diff --git a/src/amf/amfnd/compdb.cc b/src/amf/amfnd/compdb.cc index b104d5649..ebe9d1f5b 100644
--- a/src/amf/amfnd/compdb.cc
+++ b/src/amf/amfnd/compdb.cc
@@ -1706,7 +1706,7 @@ int avnd_comp_config_reinit(AVND_COMP *comp) {
   const SaImmAttrValuesT_2 **attributes;
   SaImmHandleT immOmHandle;
   SaVersionT immVersion = {'A', 2, 15};
-  SaAisErrorT error;
+  SaAisErrorT error = SA_AIS_OK;
   const SaImmAttrNameT attributeNames[] = {
     const_cast<SaImmAttrNameT>("SA_IMM_SEARCH_GET_CONFIG_ATTR"),
     nullptr};
@@ -1767,6 +1767,10 @@ done3:
 done2:
   immutil_saImmOmFinalize(immOmHandle);
 done1:
+  if (res != 0 &&
+      (error == SA_AIS_ERR_TRY_AGAIN || error == SA_AIS_ERR_TIMEOUT)) {
+    res = error;
+  }
   TRACE_LEAVE2("%u", res);
   return res;
 }
--
2.40.1

[devel] [PATCH 1/1] amf: retry if imm unavailable during recovery [#3344]

[thien.m.huynh <thi...@de...>]

If a component has changed, AMF will obtain configuration
from IMM when recovering it. If IMM is unavailable and
SC absence is enabled, AMF will immediately fail.

Retrying the cleanup event is the solution.
This is to prevent watchdog kill AMFND.
---
 src/amf/amfnd/clc.cc    | 14 +++++++++++---
 src/amf/amfnd/compdb.cc |  6 +++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/amf/amfnd/clc.cc b/src/amf/amfnd/clc.cc
index 0b0d62634..3c35a916f 100644
--- a/src/amf/amfnd/clc.cc
+++ b/src/amf/amfnd/clc.cc
@@ -1969,9 +1969,17 @@ uint32_t avnd_comp_clc_xxxing_cleansucc_hdler(AVND_CB *cb, AVND_COMP *comp) {
       "'%s': Cleanup success event in the instantiating/restarting state",
       comp->name.c_str());
   /* Refresh the component configuration, it may have changed */
-  if (!m_AVND_IS_SHUTTING_DOWN(cb) && (avnd_comp_config_reinit(comp) != 0)) {
-    rc = NCSCC_RC_FAILURE;
-    goto done;
+  if (!m_AVND_IS_SHUTTING_DOWN(cb)) {
+    int res = avnd_comp_config_reinit(comp);
+    if (res != 0) {
+      if ((res == SA_AIS_ERR_TRY_AGAIN || res == SA_AIS_ERR_TIMEOUT) &&
+          avnd_comp_clc_cmd_execute(
+              cb, comp, AVND_COMP_CLC_CMD_TYPE_CLEANUP) != NCSCC_RC_SUCCESS) {
+        TRACE("AVND_COMP_CLC_CMD_TYPE_CLEANUP retry failed");
+      }
+      rc = NCSCC_RC_FAILURE;
+      goto done;
+    }
   }
 
   /*
diff --git a/src/amf/amfnd/compdb.cc b/src/amf/amfnd/compdb.cc
index b104d5649..ebe9d1f5b 100644
--- a/src/amf/amfnd/compdb.cc
+++ b/src/amf/amfnd/compdb.cc
@@ -1706,7 +1706,7 @@ int avnd_comp_config_reinit(AVND_COMP *comp) {
   const SaImmAttrValuesT_2 **attributes;
   SaImmHandleT immOmHandle;
   SaVersionT immVersion = {'A', 2, 15};
-  SaAisErrorT error;
+  SaAisErrorT error = SA_AIS_OK;
   const SaImmAttrNameT attributeNames[] = {
     const_cast<SaImmAttrNameT>("SA_IMM_SEARCH_GET_CONFIG_ATTR"),
     nullptr};
@@ -1767,6 +1767,10 @@ done3:
 done2:
   immutil_saImmOmFinalize(immOmHandle);
 done1:
+  if (res != 0 &&
+      (error == SA_AIS_ERR_TRY_AGAIN || error == SA_AIS_ERR_TIMEOUT)) {
+    res = error;
+  }
   TRACE_LEAVE2("%u", res);
   return res;
 }
-- 
2.40.1

[devel] [PATCH 0/1] Review Request for amf: retry if imm unavailable during recovery [#3344]

[thien.m.huynh <thi...@de...>]

Summary: amf: retry if imm unavailable during recovery [#3344]
Review request for Ticket(s): 3344
Peer Reviewer(s): Dat, Thang
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3344
Base revision: 3463fa7734dab0b00365e6665eee4f896ed6268e
Personal repository: git://git.code.sf.net/u/thienhuynh/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            n
 OpenSAF services        y
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   n


Comments (indicate scope for each "y" above):
---------------------------------------------
*** EXPLAIN/COMMENT THE PATCH SERIES HERE ***

revision f8fd609eb95fa347b0f1f94cb73bbd94fac61229
Author:	thien.m.huynh <thi...@de...>
Date:	Tue, 9 Jan 2024 13:13:57 +0700

amf: retry if imm unavailable during recovery [#3344]

If a component has changed, AMF will obtain configuration
from IMM when recovering it. If IMM is unavailable and
SC absence is enabled, AMF will immediately fail.

Retrying the cleanup event is the solution.
This is to prevent watchdog kill AMFND.



Complete diffstat:
------------------
 src/amf/amfnd/clc.cc    | 14 +++++++++++---
 src/amf/amfnd/compdb.cc |  6 +++++-
 2 files changed, 16 insertions(+), 4 deletions(-)


Testing Commands:
-----------------
N/A


Testing, Expected Results:
--------------------------
N/A


Conditions of Submission:
-------------------------
ACK from reviewer


Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.

Re: [devel] [PATCH 1/1] amf: check the existing sg before create new one [#3345]

[Thang D. N. <tha...@de...>]

ACK.

-----Original Message-----
From: Thien Minh Huynh <thi...@de...> 
Sent: Friday, December 29, 2023 1:51 PM
To: Thang Duc Nguyen <tha...@de...>; Dat Tran Quoc Phan <dat...@de...>
Cc: ope...@li...; Thien Minh Huynh <thi...@de...>
Subject: [PATCH 1/1] amf: check the existing sg before create new one [#3345]

When IMM is restarted, AMF will be re-initialized.
AMF constantly adds fresh entries to the local database.
So while delete an app, it always leaves one more sg in the database.
As a consequence of this, the assertion 'app->list_of_sg == nullptr' failed.

The solution is to check for existing sg before creating new one.
---
 src/amf/amfd/sg.cc | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/amf/amfd/sg.cc b/src/amf/amfd/sg.cc index 6ed585c49..ee6d00ca8 100644
--- a/src/amf/amfd/sg.cc
+++ b/src/amf/amfd/sg.cc
@@ -274,14 +274,20 @@ static AVD_SG *sg_create(const std::string &sg_name,
 
   TRACE_ENTER2("'%s'", sg_name.c_str());
 
-  SaNameT sgtype_dn;
-  error = immutil_getAttr("saAmfSGType", attributes, 0, &sgtype_dn);
-  osafassert(error == SA_AIS_OK);
-  sgt = sgtype_db->find(Amf::to_string(&sgtype_dn));
-  osafassert(sgt);
-  sg = sg_new(sg_name, sgt->saAmfSgtRedundancyModel);
-  sg->saAmfSGType = Amf::to_string(&sgtype_dn);
-  sg->sg_type = sgt;
+  sg = sg_db->find(sg_name);
+  if (sg == nullptr) {
+    SaNameT sgtype_dn;
+    error = immutil_getAttr("saAmfSGType", attributes, 0, &sgtype_dn);
+    osafassert(error == SA_AIS_OK);
+    sgt = sgtype_db->find(Amf::to_string(&sgtype_dn));
+    osafassert(sgt);
+    sg = sg_new(sg_name, sgt->saAmfSgtRedundancyModel);
+    sg->saAmfSGType = Amf::to_string(&sgtype_dn);
+    sg->sg_type = sgt;
+  } else {
+    TRACE("already created, refreshing config...");
+    sgt = sg->sg_type;
+  }
 
   if (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSGSuHostNodeGroup"),
                       attributes, 0, &temp_name) == SA_AIS_OK) {
--
2.40.1

[devel] [PATCH 0/1] Review Request for amf: retry if imm unavailable during recovery [#3344]

[thien.m.huynh <thi...@de...>]

Summary: amf: retry if imm unavailable during recovery [#3344]
Review request for Ticket(s): 3344
Peer Reviewer(s): Dat, Thang
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3344
Base revision: b0f9985b8f37a93afa28c76ed409bf4847d6487e
Personal repository: git://git.code.sf.net/u/thienhuynh/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            n
 OpenSAF services        y
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   n


Comments (indicate scope for each "y" above):
---------------------------------------------
*** EXPLAIN/COMMENT THE PATCH SERIES HERE ***

revision 61c0fc694a58a96a8732e3a80d42384322c0849d
Author:	thien.m.huynh <thi...@de...>
Date:	Fri, 29 Dec 2023 11:15:50 +0700

amf: retry if imm unavailable during recovery [#3344]

If a component has changed, AMF will obtain configuration
from IMM when recovering it. If IMM is unavailable and
SC absence is enabled, AMF will immediately fail.

Retrying the cleanup event is the solution.
this is to prevent watch dog kill AMFND.



Complete diffstat:
------------------
 src/amf/amfnd/clc.cc    | 14 +++++++++++---
 src/amf/amfnd/compdb.cc |  4 ++--
 2 files changed, 13 insertions(+), 5 deletions(-)


Testing Commands:
-----------------
N/A


Testing, Expected Results:
--------------------------
N/A


Conditions of Submission:
-------------------------
ACK from reviewer


Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.

[devel] [PATCH 1/1] amf: retry if imm unavailable during recovery [#3344]

[thien.m.huynh <thi...@de...>]

If a component has changed, AMF will obtain configuration
from IMM when recovering it. If IMM is unavailable and
SC absence is enabled, AMF will immediately fail.

Retrying the cleanup event is the solution.
this is to prevent watch dog kill AMFND.
---
 src/amf/amfnd/clc.cc    | 14 +++++++++++---
 src/amf/amfnd/compdb.cc |  4 ++--
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/amf/amfnd/clc.cc b/src/amf/amfnd/clc.cc
index 0b0d62634..5c59c4c1a 100644
--- a/src/amf/amfnd/clc.cc
+++ b/src/amf/amfnd/clc.cc
@@ -1969,9 +1969,17 @@ uint32_t avnd_comp_clc_xxxing_cleansucc_hdler(AVND_CB *cb, AVND_COMP *comp) {
       "'%s': Cleanup success event in the instantiating/restarting state",
       comp->name.c_str());
   /* Refresh the component configuration, it may have changed */
-  if (!m_AVND_IS_SHUTTING_DOWN(cb) && (avnd_comp_config_reinit(comp) != 0)) {
-    rc = NCSCC_RC_FAILURE;
-    goto done;
+  if (!m_AVND_IS_SHUTTING_DOWN(cb)) {
+    int res = avnd_comp_config_reinit(comp);
+    if (res != 0) {
+      if (res == SA_AIS_ERR_TRY_AGAIN &&
+          avnd_comp_clc_cmd_execute(
+              cb, comp, AVND_COMP_CLC_CMD_TYPE_CLEANUP) != NCSCC_RC_SUCCESS) {
+        TRACE("AVND_COMP_CLC_CMD_TYPE_CLEANUP retry failed");
+      }
+      rc = NCSCC_RC_FAILURE;
+      goto done;
+    }
   }
 
   /*
diff --git a/src/amf/amfnd/compdb.cc b/src/amf/amfnd/compdb.cc
index b104d5649..981efda2b 100644
--- a/src/amf/amfnd/compdb.cc
+++ b/src/amf/amfnd/compdb.cc
@@ -1706,7 +1706,7 @@ int avnd_comp_config_reinit(AVND_COMP *comp) {
   const SaImmAttrValuesT_2 **attributes;
   SaImmHandleT immOmHandle;
   SaVersionT immVersion = {'A', 2, 15};
-  SaAisErrorT error;
+  SaAisErrorT error = SA_AIS_OK;
   const SaImmAttrNameT attributeNames[] = {
     const_cast<SaImmAttrNameT>("SA_IMM_SEARCH_GET_CONFIG_ATTR"),
     nullptr};
@@ -1768,7 +1768,7 @@ done2:
   immutil_saImmOmFinalize(immOmHandle);
 done1:
   TRACE_LEAVE2("%u", res);
-  return res;
+  return res != 0 && error == SA_AIS_ERR_TRY_AGAIN ? SA_AIS_ERR_TRY_AGAIN : res;
 }
 
 /**
-- 
2.40.1

[devel] [PATCH 1/1] amf: check the existing sg before create new one [#3345]

[thien.m.huynh <thi...@de...>]

When IMM is restarted, AMF will be re-initialized.
AMF constantly adds fresh entries to the local database.
So while delete an app, it always leaves one more sg in the database.
As a consequence of this, the assertion 'app->list_of_sg == nullptr' failed.

The solution is to check for existing sg before creating new one.
---
 src/amf/amfd/sg.cc | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/amf/amfd/sg.cc b/src/amf/amfd/sg.cc
index 6ed585c49..ee6d00ca8 100644
--- a/src/amf/amfd/sg.cc
+++ b/src/amf/amfd/sg.cc
@@ -274,14 +274,20 @@ static AVD_SG *sg_create(const std::string &sg_name,
 
   TRACE_ENTER2("'%s'", sg_name.c_str());
 
-  SaNameT sgtype_dn;
-  error = immutil_getAttr("saAmfSGType", attributes, 0, &sgtype_dn);
-  osafassert(error == SA_AIS_OK);
-  sgt = sgtype_db->find(Amf::to_string(&sgtype_dn));
-  osafassert(sgt);
-  sg = sg_new(sg_name, sgt->saAmfSgtRedundancyModel);
-  sg->saAmfSGType = Amf::to_string(&sgtype_dn);
-  sg->sg_type = sgt;
+  sg = sg_db->find(sg_name);
+  if (sg == nullptr) {
+    SaNameT sgtype_dn;
+    error = immutil_getAttr("saAmfSGType", attributes, 0, &sgtype_dn);
+    osafassert(error == SA_AIS_OK);
+    sgt = sgtype_db->find(Amf::to_string(&sgtype_dn));
+    osafassert(sgt);
+    sg = sg_new(sg_name, sgt->saAmfSgtRedundancyModel);
+    sg->saAmfSGType = Amf::to_string(&sgtype_dn);
+    sg->sg_type = sgt;
+  } else {
+    TRACE("already created, refreshing config...");
+    sgt = sg->sg_type;
+  }
 
   if (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSGSuHostNodeGroup"),
                       attributes, 0, &temp_name) == SA_AIS_OK) {
-- 
2.40.1

[devel] [PATCH 0/1] Review Request for amf: check the existing sg before create new one [#3345]

[thien.m.huynh <thi...@de...>]

Summary: amf: check the existing sg before create new one [#3345]
Review request for Ticket(s): 3345
Peer Reviewer(s): Dat, Thang
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3345
Base revision: b0f9985b8f37a93afa28c76ed409bf4847d6487e
Personal repository: git://git.code.sf.net/u/thienhuynh/review

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            n
 OpenSAF services        y
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   n


Comments (indicate scope for each "y" above):
---------------------------------------------
*** EXPLAIN/COMMENT THE PATCH SERIES HERE ***

revision 07b22f2ec87bcf200431da80cde850d7624de53b
Author:	thien.m.huynh <thi...@de...>
Date:	Fri, 29 Dec 2023 13:28:47 +0700

amf: check the existing sg before create new one [#3345]

When IMM is restarted, AMF will be re-initialized.
AMF constantly adds fresh entries to the local database.
So while delete an app, it always leaves one more sg in the database.
As a consequence of this, the assertion 'app->list_of_sg == nullptr' failed.

The solution is to check for existing sg before creating new one.



Complete diffstat:
------------------
 src/amf/amfd/sg.cc | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)


Testing Commands:
-----------------
N/A


Testing, Expected Results:
--------------------------
N/A

Conditions of Submission:
-------------------------
ACK from reviewer


Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          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 ~/.gitconfig file (i.e. user.name, user.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.