Re: [devel] [PATCH 1/1] pyosaf: add README for high level python interfaces [#2674]

[Long Nguyen]

Hi Hans,

Thanks for your very helpful comments. I will fix them before pushing it.

Best regards,
Long Nguyen.

On 11/20/2017 7:54 PM, Hans Nordebäck wrote:

Hi,

ack with some comments below. /Thanks HansN


On 11/10/2017 08:52 AM, Long H Buu Nguyen wrote:

---
  python/README_UTILS | 215 


  1 file changed, 215 insertions(+)
  create mode 100644 python/README_UTILS

diff --git a/python/README_UTILS b/python/README_UTILS
new file mode 100644
index 0..36316ed4c
--- /dev/null
+++ b/python/README_UTILS
@@ -0,0 +1,215 @@
+/*  -*- OpenSAF  -*-
+ *
+ * Copyright Ericsson AB 2017 - All Rights Reserved.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are 
licensed
+ * under the GNU Lesser General Public License Version 2.1, February 
1999.

+ * The complete license can be accessed from the following location:
+ * http://opensource.org/licenses/lgpl-license.php
+ * See the Copying file included with the OpenSAF distribution for full
+ * licensing terms.
+ *
+ */
+
+GENERAL
+---
+This README describes the general features and usage of the pyosaf 
utils.

+Ticket: https://sourceforge.net/p/opensaf/tickets/2602/
+
+STRUCTURE
+-
+.
+`-- pyosaf
+`-- utils
+|-- __init__.py
+|-- clm
+|   `-- __init__.py
+|-- immoi
+|   |-- __init__.py
+|   `-- implementer.py
+|-- immom
+|   |-- __init__.py
+|   |-- accessor.py
+|   |-- agent.py
+|   |-- ccb.py
+|   |-- iterator.py
+|   `-- object.py
+|-- log
+|   |-- __init__.py
+|   `-- logger.py
+`-- ntf
+|-- __init__.py
+|-- agent.py
+|-- producer.py
+|-- reader.py
+`-- subscriber.py
+
+NOTES
+-
+- pyosaf utils are refactored to take advantage of object-oriented 
programming.

+
+- The old functional interfaces are still kept for backward 
compability, but
+are marked as "deprecated", and may be removed in the next OpenSAF 
release.

+
+- pyosaf utils now return error codes instead of raising SafException.
+To keep the compability, the functions marked with "deprecated" 
still raise

+SafException in case of errors.
+
+- This README only provides a general description of how to use the 
pyosaf

+utils. Samples will be updated in order to provide more details.
+
+DESCRIPTIONS
+
+- pyosaf utils provide a self-retry error handling mechanism with some
+configurable parameters:
+
+1) MAX_RETRY_TIME: an environment variable specificing the maximum 
time to

+retry on a retriable error.

[HansN] perhaps:

1) MAX_RETRY_TIME: an environment variable specifying the maximum 
retry time

   on a recoverable error.

+A default value (60 seconds) is used if an user-defined value is not 
set

+before any pyosaf.utils modules are imported.
+


+2) RETRY_INTERVAL: an environment variable specificing the wait-time 
between

+each retry.
+A default value (1 second) is used if an user-defined value is not set
+before any pyosaf utils.modules are imported.
+

[HansN] perhaps:

2) RETRY_INTERVAL: an environment variable specifying the wait-time 
between

each retry.

+In case an error still occurs after MAX_RETRY_TIME time, pyosaf 
utils stop

+retry and return the error code to the users.

[HansN] perhaps:

In case an error still occurs after MAX_RETRY_TIME time, pyosaf utils 
stops

retrying and returns the error code to the caller.


+- The error handling is applied for these error codes:
+1) SA_AIS_ERR_TRY_AGAIN
+2) SA_AIS_ERR_NO_RESOURCES
+3) SA_AIS_ERR_BUSY
+4) SA_AIS_ERR_FAILED_OPERATION: pyosaf utils only retry in case of 
an IMM CCB

+resource-abort.
+5) SA_AIS_ERR_BAD_HANDLE: pyosaf utils retry most of BAD_HANDLE cases.
+

[HansN] change retry to retries

+CLM class descriptions:
+
+1) ClmAgent: provide CLM functions to the users at a higher level,
+and relieving the users of the need to manage the life cycle of the 
CLM agent.

+- Example:
+from pyosaf.utils.clm import ClmAgent
+clma = ClmAgent()
+clma.init()
+rc, cluster_nodes = clma.get_members()
+
+IMM-OI class descriptions:
+===
+1) Implementer (implementer.py): represent an object implementer.
+- Example:
+from pyosaf.utils.immoi.implementer import Implementer
+time_implementer = Implementer(name="TimeReporter")
+obj = ImmObject(class_name=class_name, dn=dn)
+obj.hours = hours
+obj.minutes = minutes
+obj.seconds = seconds
+obj.timeId = "timeId=1"
+time_implementer.create(obj)
+
+2) Applier (implementer.py): represent an object applier.

[HansN] represents ?

+- Example:
+from pyosaf.utils.immoi.implementer import Applier
+echo_applier = Applier(on_apply=handle_apply,
+   

Re: [devel] [PATCH 1/1] pyosaf: High level python interfaces for CLM [#2602]

[Long Nguyen]

Hi Quyen,

Thanks for your comments.
I will fix and send a new patch.

Best regards,
Long Nguyen.

On 10/27/2017 2:12 PM, Quyen Dao wrote:


Hi Long,

Please help fix the below pylint issue and find minor comments marked 
with [Quyen].


* Module pyosaf.utils
C:211,26: Using type() instead of isinstance() for a typecheck. 
(unidiomatic-typecheck)
C:219,30: Using type() instead of isinstance() for a typecheck. 
(unidiomatic-typecheck)


* Module pyosaf.utils.clm
E:253, 4: The special method '__exit__' expects 3 param(s), 0 was 
given (unexpected-special-method-signature)


Thanks,
Quyen

On 10/26/2017 3:35 PM, Long H Buu Nguyen wrote:

- Add more error handling
- Refactor clm code
- Keep raising exceptions for existing python methods
- Add __version__ attribute to utils
---
  python/pyosaf/utils/__init__.py | 263 +---
  python/pyosaf/utils/clm/__init__.py | 595 ++--
  2 files changed, 730 insertions(+), 128 deletions(-)

diff --git a/python/pyosaf/utils/__init__.py b/python/pyosaf/utils/__init__.py
index 7a1c21b79..f39c6623b 100644
--- a/python/pyosaf/utils/__init__.py
+++ b/python/pyosaf/utils/__init__.py
@@ -16,17 +16,31 @@
  #
  
  """ pyosaf common utils """
+import os
+import syslog
  import time
+import warnings
  from copy import deepcopy
+from ctypes import POINTER
  
-from pyosaf.saAis import eSaAisErrorT

+from pyosaf import saImmOm
+from pyosaf.saAis import eSaAisErrorT, SaStringT, unmarshalNullArray
  
+# Version for pyosaf utils

+__version__ = "1.0.0"
  
-TRY_AGAIN_COUNT = 60

+# The 'MAX_RETRY_TIME' and 'RETRY_INTERVAL' environment variables shall be set
+# with user-defined values BEFORE importing the pyosaf 'utils' module;
+# Otherwise, the default values for MAX_RETRY_TIME(60s) and RETRY_INTERVAL(1s)
+# will be used throughout.
+MAX_RETRY_TIME = int(os.environ.get("MAX_RETRY_TIME")) \
+if "MAX_RETRY_TIME" in os.environ else 60
+RETRY_INTERVAL = int(os.environ.get("RETRY_INTERVAL")) \
+if "RETRY_INTERVAL" in os.environ else 1
  
  
  class SafException(Exception):

-""" SAF Exception that can be printed """
+""" SAF Exception for error during executing SAF functions """
  def __init__(self, value, msg=None):
  Exception.__init__(self)
  self.value = value
@@ -44,73 +58,244 @@ def raise_saf_exception(func, error):
  raise SafException(error, error_string)
  
  
+def check_resource_abort(ccb_handle):

+""" Get error strings from IMM and check if it is a resource abort case
+
+Args:
+ccb_handle (SaImmCcbHandleT): CCB handle
+
+Returns:
+bool: 'True' if it is a resource abort case. 'False', otherwise.
+"""
+c_error_strings = POINTER(SaStringT)()
+saImmOmCcbGetErrorStrings = decorate(saImmOm.saImmOmCcbGetErrorStrings)
+
+# Get error strings
+# As soon as the ccb_handle is finalized, the strings are freed
+rc = saImmOmCcbGetErrorStrings(ccb_handle, c_error_strings)
+if rc == eSaAisErrorT.SA_AIS_OK:
+if c_error_strings:
+list_err_strings = unmarshalNullArray(c_error_strings)
+for c_error_string in list_err_strings:
+if c_error_string.startswith("IMM: Resource abort: "):
+return True
+
+return False
+
+
  def decorate(func):
  """ Decorate the given SAF function so that it retries a fixed number of
-times if needed and raises an exception if it encounters any fault other
-than SA_AIS_ERR_TRY_AGAIN.
+times for certain returned error codes during execution
+
+Args:
+func (function): The decorated SAF function
+
+Returns:
+SaAisErrorT: Return code of the decorated SAF function
  """
  
  def inner(*args):

-""" Call "function" in the lexical scope in a retry loop and raise an
-exception if it encounters any fault other than TRY_AGAIN
+""" Call the decorated function in the lexical scope in a retry loop
+
+Args:
+args (tuple): Arguments of the decorated SAF function
  """
-one_sec_sleeps = 0
-error = func(*args)
+count_sec_sleeps = 0
+
+rc = func(*args)
  
-while error == eSaAisErrorT.SA_AIS_ERR_TRY_AGAIN:

-if one_sec_sleeps == TRY_AGAIN_COUNT:
+while rc != eSaAisErrorT.SA_AIS_OK:
+
+if count_sec_sleeps >= MAX_RETRY_TIME:
  break
-time.sleep(1)
-one_sec_sleeps += 1
  
-error = func(*args)

+if rc == eSaAisErr

Re: [devel] [PATCH 1/1] amf: send oper_state when NCS SUs already instantiated [#2443]

[Long Nguyen]

Hi Praveen,

I have sent the patch modified by you for other maintainers to review.

Thanks so much,
Long Nguyen.

On 5/17/2017 10:58 AM, Long Nguyen wrote:
> Hi Praveen,
>
> Thanks for your idea.
> Yes, your are right. It is better to move the check to the event 
> handler of presence state message.
>
> Best regards,
> Long Nguyen.
>
> On 5/16/2017 12:54 PM, praveen malviya wrote:
>> Hi Long,
>>
>> This check is very generic.
>> During su restart cases, a PI SU having NPI components will send 
>> unnecessary enabled events to AMFD. When AMFD will receive this 
>> events it will try to assign this SU and can lead to assignments in 
>> other than 2N red models cases.
>> I think check should be moved to the event handler of presence state 
>> message. Attached is the patch based on this idea.
>> What do you think?
>>
>>
>> Thanks
>> Praveen
>>
>> On 28-Apr-17 9:42 AM, Long H Buu Nguyen wrote:
>>> ---
>>>   src/amf/amfnd/susm.cc | 10 ++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/src/amf/amfnd/susm.cc b/src/amf/amfnd/susm.cc
>>> index 52af63b83..04ced426d 100644
>>> --- a/src/amf/amfnd/susm.cc
>>> +++ b/src/amf/amfnd/susm.cc
>>> @@ -1608,6 +1608,16 @@ uint32_t avnd_su_pres_fsm_run(AVND_CB *cb, 
>>> AVND_SU *su, AVND_COMP *comp,
>>> /* process state change */
>>> if (prv_st != final_st)
>>>   rc = avnd_su_pres_st_chng_prc(cb, su, prv_st, final_st);
>>> +  else {
>>> +// If SU has been already instantiated, inform amfd
>>> +if (SA_AMF_PRESENCE_INSTANTIATED == final_st &&
>>> +su_all_pi_comps_instantiated(su) == true) {
>>> +  if (m_AVND_SU_OPER_STATE_IS_ENABLED(su)) {
>>> +TRACE("SU oper state is enabled");
>>> +rc = avnd_di_oper_send(cb, su, 0);
>>> +  }
>>> +}
>>> +  }
>>> done:
>>> TRACE_LEAVE2("%u", rc);
>>>
>


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

Re: [devel] [PATCH 1/1] amf: send oper_state when NCS SUs already instantiated [#2443]

[Long Nguyen]

Hi Praveen,

Thanks for your idea.
Yes, your are right. It is better to move the check to the event handler 
of presence state message.

Best regards,
Long Nguyen.

On 5/16/2017 12:54 PM, praveen malviya wrote:
> Hi Long,
>
> This check is very generic.
> During su restart cases, a PI SU having NPI components will send 
> unnecessary enabled events to AMFD. When AMFD will receive this events 
> it will try to assign this SU and can lead to assignments in other 
> than 2N red models cases.
> I think check should be moved to the event handler of presence state 
> message. Attached is the patch based on this idea.
> What do you think?
>
>
> Thanks
> Praveen
>
> On 28-Apr-17 9:42 AM, Long H Buu Nguyen wrote:
>> ---
>>   src/amf/amfnd/susm.cc | 10 ++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/src/amf/amfnd/susm.cc b/src/amf/amfnd/susm.cc
>> index 52af63b83..04ced426d 100644
>> --- a/src/amf/amfnd/susm.cc
>> +++ b/src/amf/amfnd/susm.cc
>> @@ -1608,6 +1608,16 @@ uint32_t avnd_su_pres_fsm_run(AVND_CB *cb, 
>> AVND_SU *su, AVND_COMP *comp,
>> /* process state change */
>> if (prv_st != final_st)
>>   rc = avnd_su_pres_st_chng_prc(cb, su, prv_st, final_st);
>> +  else {
>> +// If SU has been already instantiated, inform amfd
>> +if (SA_AMF_PRESENCE_INSTANTIATED == final_st &&
>> +su_all_pi_comps_instantiated(su) == true) {
>> +  if (m_AVND_SU_OPER_STATE_IS_ENABLED(su)) {
>> +TRACE("SU oper state is enabled");
>> +rc = avnd_di_oper_send(cb, su, 0);
>> +  }
>> +}
>> +  }
>> done:
>> TRACE_LEAVE2("%u", rc);
>>


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

Re: [devel] [PATCH 0/1] Review Request for amf: send oper_state when NCS SUs already instantiated [#2443]

[Long Nguyen]

Dear maintainers,

Can you please help to review the patch?

Thanks so much,
Long Nguyen.

On 5/9/2017 9:29 AM, Long Nguyen wrote:
> Hi,
>
> Have you had time to look into the patch?
>
> Best regards,
> Long Nguyen.
>
> On 4/28/2017 11:12 AM, Long H Buu Nguyen wrote:
>> Summary: amf: send oper_state when NCS SUs already instantiated [#2443]
>> Review request for Ticket(s): 2443
>> Peer Reviewer(s): AMF devs
>> Pull request to: AMF maintainers
>> Affected branch(es): develop, release
>> Development branch: ticket-2443
>> Base revision: 94fe6f2ca5c34bafc86f001807ea08ce39f60a34
>> Personal repository: git://git.code.sf.net/u/xlobung/review
>>
>> 
>> Impacted area   Impact y/n
>> 
>>   Docsn
>>   Build systemn
>>   RPM/packaging   n
>>   Configuration files n
>>   Startup scripts n
>>   SAF servicesn
>>   OpenSAF servicesy
>>   Core libraries  n
>>   Samples n
>>   Tests   n
>>   Other   n
>>
>>
>> Comments (indicate scope for each "y" above):
>> -
>>  Assume after headless, SC-1 becomes ACTIVE. Amfnd in SC-2 sends 
>> a node_up
>> message to amfd-SC-1. amfnd-SC-2 will instantiate NCS SUs in SC-2 
>> as soon
>> as amfd-SC-1 receives the node_up message. At the time NCS SUs in 
>> SC-2
>> are INSTANTIATED, if SC-1 is rebooted, amfnd-SC-2 receives 
>> NEW_ACTIVE
>> because amfd-SC-2 is set to ACTIVE by RDE. amfnd-SC-2 sends a 
>> node_up
>> message to amfd-SC-2. Later, amfnd-SC-2 continues to instantiate 
>> NCS SUs
>> in SC-2. However, the NCS SUs in SC-2 are already INSTANTIATED. 
>> amfnd-SC-2
>> does not send oper_state message to amfd-SC-2 because the NCS SU 
>> presence
>> states do not change. As a result, amf does not continue with the 
>> normal
>> startup process.
>>
>> revision 01dc86166f3ed1b9b46534092089d5bcfaf1ef57
>> Author:Long H Buu Nguyen 
>> Date:Thu, 27 Apr 2017 19:39:09 +0700
>>
>> amf: send oper_state when NCS SUs already instantiated [#2443]
>>
>>
>>
>> Complete diffstat:
>> --
>>   src/amf/amfnd/susm.cc | 10 ++
>>   1 file changed, 10 insertions(+)
>>
>>
>> Testing Commands:
>> -
>>   As described in the ticket.
>>
>>
>> Testing, Expected Results:
>> --
>>   Opensaf starts successfully.
>>
>>
>> Conditions of Submission:
>> -
>>   Ack'ed from reviewers.
>>
>>
>> Arch  Built StartedLinux distro
>> ---
>> mipsn  n
>> mips64  n  n
>> x86 n  n
>> x86_64  y  y
>> powerpc n  n
>> powerpc64   n  n
>>
>>
>> Reviewer Checklist:
>> ---
>> [Submitters: make sure that your review doesn't trigger any checkmarks!]
>>
>>
>> Your checkin has not passed review because (see checked entries):
>>
>> ___ Your RR template is generally incomplete; it has too many blank 
>> entries
>>  that need proper data filled in.
>>
>> ___ You have failed to nominate the proper persons for review and push.
>>
>> ___ Your patches do not have proper short+long header
>>
>> ___ You have grammar/spelling in your header that is unacceptable.
>>
>> ___ You have exceeded a sensible line length in your 
>> headers/comments/text.
>>
>> ___ You have failed to put in a proper Trac Ticket # into your commits.
>>
>> ___ You have incorrectly put/left internal data in your comments/files
>>  (i.e. internal bug tracking tool IDs, product names etc)
>>
>> ___ You have not given any evidence of testing beyond basic build tests.
>>  Demonstrate some level of runtime or other sanity testing.
>>
>> ___ You have ^M present in some of your files. These have to be removed.
>>
>> ___ You have needlessly changed whitespace or added whitespace crimes
>>  like trailing spaces, or spaces before tabs.
>>
>> ___ You have mixed real technical changes with whitespace and other
>>  cosmetic code cleanup changes. These have to be separate commits.
>>
>> ___ You

Re: [devel] [PATCH 0/1] Review Request for amf: send oper_state when NCS SUs already instantiated [#2443]

[Long Nguyen]

Hi,

Have you had time to look into the patch?

Best regards,
Long Nguyen.

On 4/28/2017 11:12 AM, Long H Buu Nguyen wrote:
> Summary: amf: send oper_state when NCS SUs already instantiated [#2443]
> Review request for Ticket(s): 2443
> Peer Reviewer(s): AMF devs
> Pull request to: AMF maintainers
> Affected branch(es): develop, release
> Development branch: ticket-2443
> Base revision: 94fe6f2ca5c34bafc86f001807ea08ce39f60a34
> Personal repository: git://git.code.sf.net/u/xlobung/review
>
> 
> Impacted area   Impact y/n
> 
>   Docsn
>   Build systemn
>   RPM/packaging   n
>   Configuration files n
>   Startup scripts n
>   SAF servicesn
>   OpenSAF servicesy
>   Core libraries  n
>   Samples n
>   Tests   n
>   Other   n
>
>
> Comments (indicate scope for each "y" above):
> -
>  Assume after headless, SC-1 becomes ACTIVE. Amfnd in SC-2 sends a node_up
>   message to amfd-SC-1. amfnd-SC-2 will instantiate NCS SUs in SC-2 as 
> soon
>   as amfd-SC-1 receives the node_up message. At the time NCS SUs in SC-2
>   are INSTANTIATED, if SC-1 is rebooted, amfnd-SC-2 receives NEW_ACTIVE
>   because amfd-SC-2 is set to ACTIVE by RDE. amfnd-SC-2 sends a node_up
>   message to amfd-SC-2. Later, amfnd-SC-2 continues to instantiate NCS SUs
>   in SC-2. However, the NCS SUs in SC-2 are already INSTANTIATED. 
> amfnd-SC-2
>   does not send oper_state message to amfd-SC-2 because the NCS SU 
> presence
>   states do not change. As a result, amf does not continue with the normal
>   startup process.
>
> revision 01dc86166f3ed1b9b46534092089d5bcfaf1ef57
> Author:   Long H Buu Nguyen 
> Date: Thu, 27 Apr 2017 19:39:09 +0700
>
> amf: send oper_state when NCS SUs already instantiated [#2443]
>
>
>
> Complete diffstat:
> --
>   src/amf/amfnd/susm.cc | 10 ++
>   1 file changed, 10 insertions(+)
>
>
> Testing Commands:
> -
>   As described in the ticket.
>
>
> Testing, Expected Results:
> --
>   Opensaf starts successfully.
>
>
> Conditions of Submission:
> -
>   Ack'ed from reviewers.
>
>
> Arch  Built StartedLinux distro
> ---
> mipsn  n
> mips64  n  n
> x86 n  n
> x86_64  y  y
> powerpc n  n
> powerpc64   n  n
>
>
> Reviewer Checklist:
> ---
> [Submitters: make sure that your review doesn't trigger any checkmarks!]
>
>
> Your checkin has not passed review because (see checked entries):
>
> ___ Your RR template is generally incomplete; it has too many blank entries
>  that need proper data filled in.
>
> ___ You have failed to nominate the proper persons for review and push.
>
> ___ Your patches do not have proper short+long header
>
> ___ You have grammar/spelling in your header that is unacceptable.
>
> ___ You have exceeded a sensible line length in your headers/comments/text.
>
> ___ You have failed to put in a proper Trac Ticket # into your commits.
>
> ___ You have incorrectly put/left internal data in your comments/files
>  (i.e. internal bug tracking tool IDs, product names etc)
>
> ___ You have not given any evidence of testing beyond basic build tests.
>  Demonstrate some level of runtime or other sanity testing.
>
> ___ You have ^M present in some of your files. These have to be removed.
>
> ___ You have needlessly changed whitespace or added whitespace crimes
>  like trailing spaces, or spaces before tabs.
>
> ___ You have mixed real technical changes with whitespace and other
>  cosmetic code cleanup changes. These have to be separate commits.
>
> ___ You need to refactor your submission into logical chunks; there is
>  too much content into a single commit.
>
> ___ You have extraneous garbage in your review (merge commits etc)
>
> ___ You have giant attachments which should never have been sent;
>  Instead you should place your content in a public tree to be pulled.
>
> ___ You have too many commits attached to an e-mail; resend as threaded
>  commits, or place in a public tree for a pull.
>
> ___ You have resent this content multiple times without a clear indication
>  of what has changed between each re-send.
>
> ___ You have failed to adequately and individually add

Re: [devel] [PATCH 1 of 1] amfd: fix si-swap timeout [#2190]

[Long Nguyen]

Hi Praveen,

In the trace, for example, in SC-1, we can see the code was performed like:

Nov 14 11:02:27.743481 osafamfd [486:sg.cc:1693] >> set_fsm_state
...
Nov 14 11:02:27.744077 osafamfd [486:sg.cc:1716] TR Admin operation 
finishes on SI:'safSi=SC-2N,safApp=OpenSAF'
*Nov 14 11:02:27.744090 osafamfd [486:imm.cc:1971] >> 
avd_saImmOiAdminOperationResult: inv:17179869185, res:1**
**Nov 14 11:02:27.744106 osafamfd [486:imm.cc:1976] << 
avd_saImmOiAdminOperationResult *
Nov 14 11:02:27.744119 osafamfd [486:sg.cc:1724] << set_fsm_state

The traces map to the code:
> for (const auto& si : list_of_si) {
> if (si->invocation != 0) {
> TRACE("Admin operation finishes on 
> SI:'%s'",si->name.c_str());
>  avd_saImmOiAdminOperationResult(avd_cb->immOiHandle,
> si->invocation, SA_AIS_OK);
> *si->invocation = 0;*
> }
> }

So, the invocation here is reset.

Best regards,
Long Nguyen.

On 11/17/2016 4:15 PM, praveen malviya wrote:
> Hi Long,
>
> As per ticket, issue occurred during si-swap.
> Please update the ticket with brief analysis so that seeing the ticket 
> one can have idea where the problem occurred.
> I guess si-swap initiated before AMFD could update to IMM.
> If that is the case, we should reject the admin operation from 
> si_admin_op_cb() if invocationId is still set in Si.
>
> Thanks,
> Praveen
>
> On 16-Nov-16 4:19 PM, Long HB Nguyen wrote:
>>  osaf/services/saf/amf/amfd/imm.cc |  22 ++
>>  osaf/services/saf/amf/amfd/include/imm.h |   2 ++
>>  osaf/services/saf/amf/amfd/role.cc   |   5 +
>>  3 files changed, 29 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/osaf/services/saf/amf/amfd/imm.cc 
>> b/osaf/services/saf/amf/amfd/imm.cc
>> --- a/osaf/services/saf/amf/amfd/imm.cc
>> +++ b/osaf/services/saf/amf/amfd/imm.cc
>> @@ -423,6 +423,28 @@ AvdJobDequeueResultT Fifo::execute(const
>>  return ret;
>>  }
>>
>> +AvdJobDequeueResultT Fifo::executeAdminResp(const AVD_CL_CB *cb)
>> +{
>> +Job *ajob;
>> +AvdJobDequeueResultT ret = JOB_EXECUTED;
>> +
>> +TRACE_ENTER();
>> +
>> +while ((ajob = peek()) != nullptr) {
>> +if (dynamic_cast(ajob) != nullptr) {
>> +ret = ajob->exec(cb);
>> +} else {
>> +ajob = dequeue();
>> +delete ajob;
>> +ret = JOB_EXECUTED;
>> +}
>> +}
>> +
>> +TRACE_LEAVE2("%d", ret);
>> +
>> +return ret;
>> +}
>> +
>>  //
>>  void Fifo::empty()
>>  {
>> diff --git a/osaf/services/saf/amf/amfd/include/imm.h 
>> b/osaf/services/saf/amf/amfd/include/imm.h
>> --- a/osaf/services/saf/amf/amfd/include/imm.h
>> +++ b/osaf/services/saf/amf/amfd/include/imm.h
>> @@ -146,6 +146,8 @@ public:
>>
>>  static AvdJobDequeueResultT execute(const AVD_CL_CB *cb);
>>
>> +static AvdJobDequeueResultT executeAdminResp(const AVD_CL_CB 
>> *cb);
>> +
>>  static void empty();
>>
>>  static uint32_t size();
>> diff --git a/osaf/services/saf/amf/amfd/role.cc 
>> b/osaf/services/saf/amf/amfd/role.cc
>> --- a/osaf/services/saf/amf/amfd/role.cc
>> +++ b/osaf/services/saf/amf/amfd/role.cc
>> @@ -766,6 +766,11 @@ void avd_mds_qsd_role_evh(AVD_CL_CB *cb,
>>  }
>>
>>  try_again:
>> +/* Execute admin op jobs before calling saImmOiImplementerClear 
>> to avoid
>> + * SA_AIS_ERR_TIMEOUT
>> + */
>> +Fifo::executeAdminResp(cb);
>> +
>>  /* Take mutex here to sync with imm reinit thread.*/
>>  osaf_mutex_lock_ordie(&imm_reinit_mutex);
>>  /* Give up IMM OI implementer role */
>>
>

--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfd: create node if not exist [#2150]

[Long Nguyen]

Hi Praveen,

I have tested with the patch for #2112. With the patch for #2112, I am 
not able to reproduce the problem anymore.
I think we can ignore this ticket.

Thanks so much,
Long Nguyen.

On 11/10/2016 11:23 AM, Long Nguyen wrote:
> Hi Praveen,
>
> Thanks for your information. I will recheck it.
>
> Best regards,
> Long Nguyen.
>
> On 11/10/2016 11:18 AM, praveen malviya wrote:
>> Hi,
>> Thanks for the information.
>> I think, like #2175, this issue may not occur after #2112 being 
>> pushed. Can you please recheck?
>>
>> Thanks,
>> Praveen
>>
>> On 10-Nov-16 9:38 AM, Long Nguyen wrote:
>>> Hi Praveen,
>>>
>>> Yes, I think the PL-6 was added after avd_node_config_get() is 
>>> called so
>>> SC-2 did not have the info about PL-6.
>>>
>>> Best regards,
>>> Long Nguyen.
>>>
>>> On 11/8/2016 6:50 PM, praveen malviya wrote:
>>>> Hi Long,
>>>>
>>>> I have gone through the traces from both the AMFDs. I think there is a
>>>> time difference between the controllers. I just wanted to see whether
>>>> SC-2 got any CCBs for PL-6 addition. At the same time SC-2 did not get
>>>> PL-6 information in avd_node_config_get() also.
>>>> I guess PL-6 was added when SC-2 completed at-least reading
>>>> node_configuration and has not done applier set.
>>>> Is this the state of SC-2 when PL-6 was added?
>>>>
>>>>
>>>> Thanks,
>>>> Praveen
>>>>
>>>> On 02-Nov-16 9:28 AM, Long HB Nguyen wrote:
>>>>>  osaf/services/saf/amf/amfd/ckpt_updt.cc |  14 +-
>>>>>  1 files changed, 9 insertions(+), 5 deletions(-)
>>>>>
>>>>>
>>>>> diff --git a/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>>> b/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>>> --- a/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>>> +++ b/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>>> @@ -20,6 +20,7 @@
>>>>>  #include 
>>>>>  #include 
>>>>>  #include 
>>>>> +#include 
>>>>>
>>>>>  static char *action_name[] = {
>>>>>  const_cast("invalid"),
>>>>> @@ -53,10 +54,14 @@ uint32_t avd_ckpt_node(AVD_CL_CB *cb, AV
>>>>>
>>>>>  osafassert (action == NCS_MBCSV_ACT_UPDATE);
>>>>>
>>>>> -if (nullptr == (node = avd_node_get(ckpt_node->name))) {
>>>>> -LOG_WA("avd_node_get FAILED for '%s'",
>>>>> ckpt_node->name.c_str());
>>>>> -rc = NCSCC_RC_FAILURE;
>>>>> -goto done;
>>>>> +node = avd_node_get(ckpt_node->name);
>>>>> +if (node == nullptr) {
>>>>> +TRACE("'%s' does not exist, creating it",
>>>>> ckpt_node->name.c_str());
>>>>> +node = avd_node_new(ckpt_node->name);
>>>>> +node->cluster = avd_cluster;
>>>>> +node->admin_ng = nullptr;
>>>>> +rc = node_name_db->insert(node->name, node);
>>>>> +osafassert(rc == NCSCC_RC_SUCCESS);
>>>>>  }
>>>>>  /* Update all runtime attributes */
>>>>>  node->saAmfNodeAdminState = ckpt_node->saAmfNodeAdminState;
>>>>> @@ -72,7 +77,6 @@ uint32_t avd_ckpt_node(AVD_CL_CB *cb, AV
>>>>>  if (nullptr == 
>>>>> avd_node_find_nodeid(ckpt_node->node_info.nodeId))
>>>>>  avd_node_add_nodeid(node);
>>>>>
>>>>> -done:
>>>>>  TRACE_LEAVE2("%u", rc);
>>>>>  return rc;
>>>>>  }
>>>>>
>>>>
>>>
>>
>


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfd: create node if not exist [#2150]

[Long Nguyen]

Hi Praveen,

Thanks for your information. I will recheck it.

Best regards,
Long Nguyen.

On 11/10/2016 11:18 AM, praveen malviya wrote:
> Hi,
> Thanks for the information.
> I think, like #2175, this issue may not occur after #2112 being 
> pushed. Can you please recheck?
>
> Thanks,
> Praveen
>
> On 10-Nov-16 9:38 AM, Long Nguyen wrote:
>> Hi Praveen,
>>
>> Yes, I think the PL-6 was added after avd_node_config_get() is called so
>> SC-2 did not have the info about PL-6.
>>
>> Best regards,
>> Long Nguyen.
>>
>> On 11/8/2016 6:50 PM, praveen malviya wrote:
>>> Hi Long,
>>>
>>> I have gone through the traces from both the AMFDs. I think there is a
>>> time difference between the controllers. I just wanted to see whether
>>> SC-2 got any CCBs for PL-6 addition. At the same time SC-2 did not get
>>> PL-6 information in avd_node_config_get() also.
>>> I guess PL-6 was added when SC-2 completed at-least reading
>>> node_configuration and has not done applier set.
>>> Is this the state of SC-2 when PL-6 was added?
>>>
>>>
>>> Thanks,
>>> Praveen
>>>
>>> On 02-Nov-16 9:28 AM, Long HB Nguyen wrote:
>>>>  osaf/services/saf/amf/amfd/ckpt_updt.cc |  14 +-
>>>>  1 files changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>>
>>>> diff --git a/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>> b/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>> --- a/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>> +++ b/osaf/services/saf/amf/amfd/ckpt_updt.cc
>>>> @@ -20,6 +20,7 @@
>>>>  #include 
>>>>  #include 
>>>>  #include 
>>>> +#include 
>>>>
>>>>  static char *action_name[] = {
>>>>  const_cast("invalid"),
>>>> @@ -53,10 +54,14 @@ uint32_t avd_ckpt_node(AVD_CL_CB *cb, AV
>>>>
>>>>  osafassert (action == NCS_MBCSV_ACT_UPDATE);
>>>>
>>>> -if (nullptr == (node = avd_node_get(ckpt_node->name))) {
>>>> -LOG_WA("avd_node_get FAILED for '%s'",
>>>> ckpt_node->name.c_str());
>>>> -rc = NCSCC_RC_FAILURE;
>>>> -goto done;
>>>> +node = avd_node_get(ckpt_node->name);
>>>> +if (node == nullptr) {
>>>> +TRACE("'%s' does not exist, creating it",
>>>> ckpt_node->name.c_str());
>>>> +node = avd_node_new(ckpt_node->name);
>>>> +node->cluster = avd_cluster;
>>>> +node->admin_ng = nullptr;
>>>> +rc = node_name_db->insert(node->name, node);
>>>> +osafassert(rc == NCSCC_RC_SUCCESS);
>>>>  }
>>>>  /* Update all runtime attributes */
>>>>  node->saAmfNodeAdminState = ckpt_node->saAmfNodeAdminState;
>>>> @@ -72,7 +77,6 @@ uint32_t avd_ckpt_node(AVD_CL_CB *cb, AV
>>>>  if (nullptr == avd_node_find_nodeid(ckpt_node->node_info.nodeId))
>>>>  avd_node_add_nodeid(node);
>>>>
>>>> -done:
>>>>  TRACE_LEAVE2("%u", rc);
>>>>  return rc;
>>>>  }
>>>>
>>>
>>
>


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfd: create node if not exist [#2150]

[Long Nguyen]

Hi Praveen,

Yes, I think the PL-6 was added after avd_node_config_get() is called so 
SC-2 did not have the info about PL-6.

Best regards,
Long Nguyen.

On 11/8/2016 6:50 PM, praveen malviya wrote:
> Hi Long,
>
> I have gone through the traces from both the AMFDs. I think there is a 
> time difference between the controllers. I just wanted to see whether 
> SC-2 got any CCBs for PL-6 addition. At the same time SC-2 did not get 
> PL-6 information in avd_node_config_get() also.
> I guess PL-6 was added when SC-2 completed at-least reading 
> node_configuration and has not done applier set.
> Is this the state of SC-2 when PL-6 was added?
>
>
> Thanks,
> Praveen
>
> On 02-Nov-16 9:28 AM, Long HB Nguyen wrote:
>>  osaf/services/saf/amf/amfd/ckpt_updt.cc |  14 +-
>>  1 files changed, 9 insertions(+), 5 deletions(-)
>>
>>
>> diff --git a/osaf/services/saf/amf/amfd/ckpt_updt.cc 
>> b/osaf/services/saf/amf/amfd/ckpt_updt.cc
>> --- a/osaf/services/saf/amf/amfd/ckpt_updt.cc
>> +++ b/osaf/services/saf/amf/amfd/ckpt_updt.cc
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  static char *action_name[] = {
>>  const_cast("invalid"),
>> @@ -53,10 +54,14 @@ uint32_t avd_ckpt_node(AVD_CL_CB *cb, AV
>>
>>  osafassert (action == NCS_MBCSV_ACT_UPDATE);
>>
>> -if (nullptr == (node = avd_node_get(ckpt_node->name))) {
>> -LOG_WA("avd_node_get FAILED for '%s'", 
>> ckpt_node->name.c_str());
>> -rc = NCSCC_RC_FAILURE;
>> -goto done;
>> +node = avd_node_get(ckpt_node->name);
>> +if (node == nullptr) {
>> +TRACE("'%s' does not exist, creating it", 
>> ckpt_node->name.c_str());
>> +node = avd_node_new(ckpt_node->name);
>> +node->cluster = avd_cluster;
>> +node->admin_ng = nullptr;
>> +rc = node_name_db->insert(node->name, node);
>> +osafassert(rc == NCSCC_RC_SUCCESS);
>>  }
>>  /* Update all runtime attributes */
>>  node->saAmfNodeAdminState = ckpt_node->saAmfNodeAdminState;
>> @@ -72,7 +77,6 @@ uint32_t avd_ckpt_node(AVD_CL_CB *cb, AV
>>  if (nullptr == avd_node_find_nodeid(ckpt_node->node_info.nodeId))
>>  avd_node_add_nodeid(node);
>>
>> -done:
>>  TRACE_LEAVE2("%u", rc);
>>  return rc;
>>  }
>>
>


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfd: fix incorrect ER messages in sg_2n_fsm.cc [#1897]

[Long Nguyen]

Hi Praveen,

Very sorry. I somehow missed the mail. I have found your mail now. 
Thanks so much.

Best regards,
Long Nguyen.

On 10/18/2016 2:01 PM, praveen malviya wrote:
> Hi Long,
>
> I had already acked it on 14th Oct.
>
>
> Thanks,
> Praveen
>
> On 18-Oct-16 12:28 PM, Long Nguyen wrote:
>> Hi Praveen and Nagu,
>>
>> Did you have a chance to look at the patch?
>>
>> Best regards,
>> Long Nguyen.
>>
>> On 10/10/2016 11:27 AM, Gary Lee wrote:
>>> Hi Long
>>>
>>> ack
>>>
>>> Thanks
>>>
>>>> On 10 Oct. 2016, at 3:26 pm, Long HB Nguyen
>>>>  wrote:
>>>>
>>>> osaf/services/saf/amf/amfd/sg_2n_fsm.cc |  10 +-
>>>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>>
>>>> diff --git a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>>>> b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>>>> --- a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>>>> +++ b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>>>> @@ -745,21 +745,21 @@ SaAisErrorT SG_2N::si_swap(AVD_SI *si, S
>>>> TRACE_ENTER2("'%s' sg_fsm_state=%u", si->name.c_str(),
>>>> si->sg_of_si->sg_fsm_state);
>>>>
>>>> if (si->saAmfSIAdminState != SA_AMF_ADMIN_UNLOCKED) {
>>>> -LOG_ER("%s SWAP failed - wrong admin state=%u",
>>>> si->name.c_str(),
>>>> +LOG_NO("%s SWAP failed - wrong admin state=%u",
>>>> si->name.c_str(),
>>>> si->saAmfSIAdminState);
>>>> rc = SA_AIS_ERR_TRY_AGAIN;
>>>> goto done;
>>>> }
>>>>
>>>> if (avd_cb->init_state != AVD_APP_STATE) {
>>>> -LOG_ER("%s SWAP failed - not in app state (%u)",
>>>> si->name.c_str(),
>>>> +LOG_NO("%s SWAP failed - not in app state (%u)",
>>>> si->name.c_str(),
>>>> avd_cb->init_state);
>>>> rc = SA_AIS_ERR_TRY_AGAIN;
>>>> goto done;
>>>> }
>>>>
>>>> if (si->sg_of_si->sg_fsm_state != AVD_SG_FSM_STABLE) {
>>>> -LOG_ER("%s SWAP failed - SG not stable (%u)", 
>>>> si->name.c_str(),
>>>> +LOG_NO("%s SWAP failed - SG not stable (%u)", 
>>>> si->name.c_str(),
>>>> si->sg_of_si->sg_fsm_state);
>>>> rc = SA_AIS_ERR_TRY_AGAIN;
>>>> goto done;
>>>> @@ -774,14 +774,14 @@ SaAisErrorT SG_2N::si_swap(AVD_SI *si, S
>>>> if ((si->sg_of_si->sg_ncs_spec) &&
>>>> ((avd_cb->node_id_avd_other != 0) &&
>>>> (avd_cb->other_avd_adest != 0))) {
>>>> if (avd_cb->stby_sync_state == AVD_STBY_OUT_OF_SYNC) {
>>>> -LOG_ER("%s SWAP failed - Cold sync in progress",
>>>> si->name.c_str());
>>>> +LOG_NO("%s SWAP failed - Cold sync in progress",
>>>> si->name.c_str());
>>>> rc = SA_AIS_ERR_TRY_AGAIN;
>>>> goto done;
>>>> }
>>>> }
>>>>
>>>> if (si->list_of_sisu->si_next == nullptr) {
>>>> -LOG_ER("%s SWAP failed - only one assignment",
>>>> si->name.c_str());
>>>> +LOG_NO("%s SWAP failed - only one assignment",
>>>> si->name.c_str());
>>>> rc = SA_AIS_ERR_TRY_AGAIN;
>>>> goto done;
>>>> }
>>>
>>
>


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

Re: [devel] [PATCH 1 of 1] amfd: fix incorrect ER messages in sg_2n_fsm.cc [#1897]

[Long Nguyen]

Hi Praveen and Nagu,

Did you have a chance to look at the patch?

Best regards,
Long Nguyen.

On 10/10/2016 11:27 AM, Gary Lee wrote:
> Hi Long
>
> ack
>
> Thanks
>
>> On 10 Oct. 2016, at 3:26 pm, Long HB Nguyen  
>> wrote:
>>
>> osaf/services/saf/amf/amfd/sg_2n_fsm.cc |  10 +-
>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>
>>
>> diff --git a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc 
>> b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>> --- a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>> +++ b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
>> @@ -745,21 +745,21 @@ SaAisErrorT SG_2N::si_swap(AVD_SI *si, S
>>  TRACE_ENTER2("'%s' sg_fsm_state=%u", si->name.c_str(), 
>> si->sg_of_si->sg_fsm_state);
>>
>>  if (si->saAmfSIAdminState != SA_AMF_ADMIN_UNLOCKED) {
>> -LOG_ER("%s SWAP failed - wrong admin state=%u", 
>> si->name.c_str(),
>> +LOG_NO("%s SWAP failed - wrong admin state=%u", 
>> si->name.c_str(),
>>  si->saAmfSIAdminState);
>>  rc = SA_AIS_ERR_TRY_AGAIN;
>>  goto done;
>>  }
>>
>>  if (avd_cb->init_state != AVD_APP_STATE) {
>> -LOG_ER("%s SWAP failed - not in app state (%u)", 
>> si->name.c_str(),
>> +LOG_NO("%s SWAP failed - not in app state (%u)", 
>> si->name.c_str(),
>>  avd_cb->init_state);
>>  rc = SA_AIS_ERR_TRY_AGAIN;
>>  goto done;
>>  }
>>
>>  if (si->sg_of_si->sg_fsm_state != AVD_SG_FSM_STABLE) {
>> -LOG_ER("%s SWAP failed - SG not stable (%u)", si->name.c_str(),
>> +LOG_NO("%s SWAP failed - SG not stable (%u)", si->name.c_str(),
>>  si->sg_of_si->sg_fsm_state);
>>  rc = SA_AIS_ERR_TRY_AGAIN;
>>  goto done;
>> @@ -774,14 +774,14 @@ SaAisErrorT SG_2N::si_swap(AVD_SI *si, S
>>  if ((si->sg_of_si->sg_ncs_spec) &&
>>  ((avd_cb->node_id_avd_other != 0) && 
>> (avd_cb->other_avd_adest != 0))) {
>>  if (avd_cb->stby_sync_state == AVD_STBY_OUT_OF_SYNC) {
>> -LOG_ER("%s SWAP failed - Cold sync in progress", 
>> si->name.c_str());
>> +LOG_NO("%s SWAP failed - Cold sync in progress", 
>> si->name.c_str());
>>  rc = SA_AIS_ERR_TRY_AGAIN;
>>  goto done;
>>  }
>>  }
>>
>>  if (si->list_of_sisu->si_next == nullptr) {
>> -LOG_ER("%s SWAP failed - only one assignment", 
>> si->name.c_str());
>> +LOG_NO("%s SWAP failed - only one assignment", 
>> si->name.c_str());
>>  rc = SA_AIS_ERR_TRY_AGAIN;
>>  goto done;
>>  }
>


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

Re: [devel] [PATCH 1 of 1] amfa: fix pg track returns SA_AIS_ERR_INIT [#1998]

[Long Nguyen]

Thanks Praveen,

> Will this fix #1991 also? 
No, it won't.

Best regards,
Long Nguyen.

On 9/6/2016 12:40 PM, praveen malviya wrote:
> Ack, code review only.
>
> Will this fix #1991 also?
>
> Thanks,
> Praveen
>
> On 05-Sep-16 5:46 PM, Long HB Nguyen wrote:
>>  osaf/libs/agents/saf/amfa/include/ava_hdl.h |  3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>>
>> diff --git a/osaf/libs/agents/saf/amfa/include/ava_hdl.h 
>> b/osaf/libs/agents/saf/amfa/include/ava_hdl.h
>> --- a/osaf/libs/agents/saf/amfa/include/ava_hdl.h
>> +++ b/osaf/libs/agents/saf/amfa/include/ava_hdl.h
>> @@ -175,7 +175,8 @@ typedef struct ava_hdl_db_tag {
>>
>>  /* Macro to determine if pg callback was supplied during 
>> saAmfInitialize() */
>>  #define m_AVA_HDL_IS_PG_CBK_PRESENT(hdl_rec) \
>> -   ( hdl_rec->reg_cbk.saAmfProtectionGroupTrackCallback )
>> +   ( hdl_rec->reg_cbk.saAmfProtectionGroupTrackCallback  || \
>> + hdl_rec->reg_cbk.saAmfProtectionGroupTrackCallback_4)
>>
>>  /* define all flags here */
>>  #define AVA_HDL_CBK_RESP_DONE0x0001
>>
>


--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] Review Request for amf: update PR doc for long DN support [#1642]

[Long Nguyen]

Hi Praveen,

Do you have any comments for the PR?

/_Note:_/ I will remove the internal changes in amfd and amfnd later.

Best regards,
Long Nguyen.

On 8/23/2016 2:26 PM, Long Nguyen wrote:
> Summary: amf: update PR doc for long DN support [#1642]
> Review request for Trac Ticket(s): #1642
> Peer Reviewer(s): AMF devs
> Pull request to: AMF maintainers
> Affected branch(es): default
> Development branch: default
>
> 
> Impacted area   Impact y/n
> 
>  Docsy
>  Build systemn
>  RPM/packaging   n
>  Configuration files n
>  Startup scripts n
>  SAF servicesn
>  OpenSAF servicesn
>  Core libraries  n
>  Samples n
>  Tests   n
>  Other   n
>
>
> Comments (indicate scope for each "y" above):
> -
>  <>
>
> amf: update PR doc for long DN support [#1642]
>
> 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.
>

--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfa: fixed freeing notification buff [#1642]

[Long Nguyen]

Hi Minh and Praveen,

I have just sent out the patch (amfa: fix memory leak in protection group).
Please help to review. Thanks so much.

Best regards,
Long Nguyen.

On 8/26/2016 12:47 PM, minh.c...@dektech.com.au wrote:
> Hi Praveen,
>
> Long is preparing the patch adding sentinel element.
>
> Thanks,
> Minh
>
>> Hi Minh,
>>
>> I think this is subject to interpretation. After finalize(), handle
>> becomes invalid. So an application cannot call Free_4() to free any
>> memory. In such a case, freeing any resources associated with this
>> handle in finalize() seems to be ok.
>>Anyways freeing in finalize() can be postponed for any real use case
>> to come up.In that case please go ahead by adding sentinel element.
>> I think there are not other pending things in #1642 other than this.
>>
>> Thanks,
>> Praveen
>>
>>
>>
>> On 26-Aug-16 7:35 AM, minh chau wrote:
>>> Hi Praveen,
>>>
>>> Just to confirm if I understand correctly the problem you mentioned in
>>> saAmfFinalize(). As the specification says application should call
>>> free()/Free_4() to release the allocated memory, if application does not
>>> release memory then it's most likely application misuses API. Or do you
>>> mean saAmfFinalize() should do the same as saNtfFinalize()? if this is
>>> the case it looks just an enhancement to guard AMFA?
>>>
>>> Thanks
>>> Minh
>>>
>>> On 25/08/16 20:04, praveen malviya wrote:
>>>> Hi Minh,
>>>>
>>>> AMFA currently does not remember the allocated memory. It relies on
>>>> the application always to free the memory. In saAmfFinalize() also, it
>>>> does not free the memory. I think AMFA should remember memory by
>>>> associating it with handle because process which is starting PG
>>>> tracking may not be a component and may not call free()/Free_4() and
>>>> just relies on saAmfFinalize() call to release all the resources.
>>>>
>>>> I think as of now please go ahead with your suggested solution. From
>>>> finalize perspective this is a defect and applicable to all the
>>>> branches. So please raise a ticket for that.
>>>>
>>>> Thanks,
>>>> Praveen
>>>>
>>>>
>>>> On 22-Aug-16 12:08 PM, minh chau wrote:
>>>>> Hi Praveen,
>>>>>
>>>>> The case you just mentioned is still in callback context, so Agent can
>>>>> help application to release the allocated notification. But still
>>>>> another case:
>>>>>
>>>>> +SaAmfProtectionGroupNotificationBufferT buff;
>>>>> +buff.notification = NULL;
>>>>> +rc = saAmfProtectionGroupTrack_4(my_amf_hdl, &track_csi,
>>>>> SA_TRACK_CURRENT, &buff);
>>>>> +if (rc != SA_AIS_OK) {
>>>>> +syslog(LOG_ERR, "saAmfProtectionGroupTrack FAILED - %u", rc);
>>>>> +goto done;
>>>>> +}
>>>>>
>>>>> In this case Agent has to allocate notification but it's not in
>>>>> Agent's
>>>>> context.
>>>>> Application has to call API Free_4(buff.notification) to free up
>>>>> notification.
>>>>> In order to iterate to free longDn(s) inside Free_4(), Agent has to
>>>>> memorize a list numberOfItems for every single call as above
>>>>> Track_4(),
>>>>> or Agent can add sentinel element to the allocated notification.
>>>>>
>>>>> Thanks,
>>>>> Minh
>>>>>
>>>>> On 22/08/16 15:34, praveen malviya wrote:
>>>>>> Hi,
>>>>>> The callback looks like this:
>>>>>> typedef void
>>>>>> (*SaAmfProtectionGroupTrackCallbackT_4)(
>>>>>>  const SaNameT *csiName,
>>>>>>  SaAmfProtectionGroupNotificationBufferT_4 *notificationBuffer,
>>>>>>  SaUint32T numberOfMembers,
>>>>>>  SaAisErrorT error);
>>>>>>
>>>>>> Inside this callback, application is supposed to call
>>>>>> saAmfProtectionGroupNotificationFree_4(). So agent must be able to
>>>>>> deduce this information as SaAmfProtectionGroupNotificationBufferT_4
>>>>>> contains numberOfItems and also numberOfMembers is available from
>>>>>> callback.
>>>>>> Since B.

Re: [devel] [PATCH 1 of 1] amf: README file for long DN support [#1642]

[Long Nguyen]

Thanks Praveen,

Actually, the PR doc only contains the implementation detail part in 
readme file.
I will remove the internal amfd and amfnd changes in the PR doc.

Best regards,
Long Nguyen.

On 8/25/2016 11:42 AM, praveen malviya wrote:
> Ack.
>
> I think implementation details should go in PR doc also(not to mention 
> internal amfd and amfnd changes).
>
> Thanks,
> Praveen
>
> On 23-Aug-16 12:44 PM, Long HB Nguyen wrote:
>>  osaf/services/saf/amf/README |  72 
>> 
>>  1 files changed, 72 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/osaf/services/saf/amf/README b/osaf/services/saf/amf/README
>> new file mode 100755
>> --- /dev/null
>> +++ b/osaf/services/saf/amf/README
>> @@ -0,0 +1,72 @@
>> +#
>> +#  -*- OpenSAF  -*-
>> +#
>> +# (C) Copyright 2016 The OpenSAF Foundation
>> +#
>> +# This program is distributed in the hope that it will be useful, but
>> +# WITHOUT ANY WARRANTY; without even the implied warranty of 
>> MERCHANTABILITY
>> +# or FITNESS FOR A PARTICULAR PURPOSE. This file and program are 
>> licensed
>> +# under the GNU Lesser General Public License Version 2.1, February 
>> 1999.
>> +# The complete license can be accessed from the following location:
>> +# http://opensource.org/licenses/lgpl-license.php
>> +# See the Copying file included with the OpenSAF distribution for full
>> +# licensing terms.
>> +#
>> +# Author(s): Ericsson AB
>> +#
>> +
>> +GENERAL
>> +---
>> +
>> +This is a description of how the AMF service supports long DNs.
>> +Ticket: https://sourceforge.net/p/opensaf/tickets/1642
>> +This enhancement is part of the general enhancement for supporting long
>> +DNs in OpenSAF introduced by ticket: #191. Ticket #1642 adds support 
>> for
>> +long DNs to AMF.
>> +
>> +CONFIGURATION
>> +-
>> +
>> +To enable support for long DNs/RDNs in IMM, the longDnsAllowed
>> +attribute must be enabled using the following command:
>> +
>> +immcfg -a longDnsAllowed=1 opensafImm=opensafImm,safApp=safImmService
>> +
>> +The IMM service will reject attempts to set the longDnsAllowed 
>> attribute
>> +back to 0 if the IMM database contains objects with DNs longer than 
>> 255 bytes
>> +or RDNs longer than 64 bytes.
>> +
>> +To enable long DN support, an application must follow these things:
>> +- Compile the application with the SA_EXTENDED_NAME_SOURCE preprocessor
>> +macro defined.
>> +- Set the environment variable SA_ENABLE_EXTENDED_NAMES to 1 before
>> +the first call to any SAF API.
>> +- Treat the SaNameT type as opaque and use the SaNameT tunnelling 
>> primitives
>> +saAisNameLend(), saAisNameBorrow() to manipulate SaNameT variables.
>> +
>> +IMPLEMENTATION DETAILS
>> +--
>> +
>> +The implementation has been adapted to the "tunnelling" solution 
>> provided
>> +by the OpenSAF generic patch (#191).
>> +Please see the document below on how to use the SaNameT tunnelling 
>> primitives:
>> +OpenSAF_Extensions_PR.odt
>> +
>> +This implementation also includes following enhancements:
>> +1) Replace internal SaNameT variables with std::string variables in 
>> amfd, amfnd.
>> +2) Replace patricia trees with std::map in amfnd.
>> +
>> +saAmfDispatch() may return SA_AIS_ERR_NAME_TOO_LONG if an 
>> application which
>> +does not support long DN, receives an AMF callback containing a long 
>> DN entity.
>> +An application should not be configured with long DN entities until 
>> long DN
>> +support has been added.
>
>> +
>> +In AMF B.01.01 spec, the application can free extended SaNameT 
>> variables in
>> +SaAmfProtectionGroupNotificationBufferT using the example below:
>> +SaAmfProtectionGroupNotificationBufferT buff;
>> +...
>> +for ( i=0; i> +if 
>> (strlen(saAisNameBorrow(buff.notification[i].member.comp_name)) >
>> +SA_MAX_UNEXTENDED_NAME_LENGTH)
>> + free(saAisNameBorrow(buff.notification[i].member.comp_name));
>> +}
>>
>



--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfd: adapt unit tests for long DN support [#1642]

[Long Nguyen]

Hi Gary,

Ack (Tested).

Best regards,
Long Nguyen.

On 8/24/2016 10:47 AM, Gary Lee wrote:
>   osaf/services/saf/amf/amfd/tests/test_amfdb.cc|   2 +-
>   osaf/services/saf/amf/amfd/tests/test_ckpt_enc_dec.cc |  46 
> +++---
>   2 files changed, 19 insertions(+), 29 deletions(-)
>
>
> diff --git a/osaf/services/saf/amf/amfd/tests/test_amfdb.cc 
> b/osaf/services/saf/amf/amfd/tests/test_amfdb.cc
> --- a/osaf/services/saf/amf/amfd/tests/test_amfdb.cc
> +++ b/osaf/services/saf/amf/amfd/tests/test_amfdb.cc
> @@ -16,7 +16,7 @@
>*/
>   
>   #include "gtest/gtest.h"
> -#include "db_template.h"
> +#include "amf_db_template.h"
>   
>   class TEST_APP {
>   };
> diff --git a/osaf/services/saf/amf/amfd/tests/test_ckpt_enc_dec.cc 
> b/osaf/services/saf/amf/amfd/tests/test_ckpt_enc_dec.cc
> --- a/osaf/services/saf/amf/amfd/tests/test_ckpt_enc_dec.cc
> +++ b/osaf/services/saf/amf/amfd/tests/test_ckpt_enc_dec.cc
> @@ -39,12 +39,6 @@ class CkptEncDecTest : public ::testing:
> virtual void TearDown() {
> }
>   
> -  const SaNameT* asSaNameT(const std::string& name) {
> -sa_name_t.length = name.size();
> -memcpy((char*)sa_name_t.value, name.c_str(), sa_name_t.length);
> -return &sa_name_t;
> -  }
> -
> bool isLittleEndian() const {
>   union {
> int i;
> @@ -58,7 +52,6 @@ class CkptEncDecTest : public ::testing:
>   }
> }
>   
> -  SaNameT sa_name_t {};
> NCS_MBCSV_CB_DEC dec {};
> NCS_MBCSV_CB_ENC enc {};
> NCS_UBAID uba {};
> @@ -67,7 +60,7 @@ class CkptEncDecTest : public ::testing:
>   TEST_F(CkptEncDecTest, testEncDecAvdApp) {
> int rc = 0;
> std::string app_name {"AppName"};
> -  AVD_APP app(asSaNameT(app_name));
> +  AVD_APP app(app_name);
> app.saAmfApplicationAdminState = SA_AMF_ADMIN_LOCKED;
> app.saAmfApplicationCurrNumSGs = 0x44332211;
>   
> @@ -81,7 +74,7 @@ TEST_F(CkptEncDecTest, testEncDecAvdApp)
> enc.i_peer_version = AVD_MBCSV_SUB_PART_VERSION_3;
>   
> encode_app(&enc.io_uba, &app);
> -
> +
> // retrieve saAmfApplicationCurrNumSGs encoded from the USR buf
> int32_t size = enc.io_uba.ttl;
> char *tmpData = new char[size];
> @@ -99,11 +92,10 @@ TEST_F(CkptEncDecTest, testEncDecAvdApp)
> }
>   
> delete [] tmpData;
> -
> -  memset(&app, '\0', sizeof(AVD_APP));
> +
> decode_app(&enc.io_uba, &app);
>   
> -  ASSERT_EQ(Amf::to_string(&app.name), "AppName");
> +  ASSERT_EQ(app.name, "AppName");
> ASSERT_EQ(app.saAmfApplicationAdminState, SA_AMF_ADMIN_LOCKED);
> ASSERT_EQ(app.saAmfApplicationCurrNumSGs, 
> static_cast(0x44332211));
>   }
> @@ -112,13 +104,13 @@ TEST_F(CkptEncDecTest, testEncDecAvdComp
> int rc = 0;
> std::string comp_name{"CompName"};
> std::string comp_proxy_name{"CompProxyName"};
> -  AVD_COMP comp(asSaNameT(comp_name));
> +  AVD_COMP comp(comp_name);
>   
> comp.saAmfCompOperState = static_cast(0x44332211);
> comp.saAmfCompReadinessState = 
> static_cast(0x55443322);
> comp.saAmfCompPresenceState = 
> static_cast(0x66554433);
> comp.saAmfCompRestartCount = 0x77665544;
> -  comp.saAmfCompCurrProxyName = *(asSaNameT(comp_proxy_name));
> +  comp.saAmfCompCurrProxyName = comp_proxy_name;
>   
> rc = ncs_enc_init_space(&enc.io_uba);
> ASSERT_TRUE(rc == NCSCC_RC_SUCCESS);
> @@ -130,7 +122,7 @@ TEST_F(CkptEncDecTest, testEncDecAvdComp
> enc.i_peer_version = AVD_MBCSV_SUB_PART_VERSION_3;
>   
> encode_comp(&enc.io_uba, &comp);
> -
> +
> // retrieve AVD_COMP encoded data from the USR buf
> int32_t size = enc.io_uba.ttl;
> char *tmpData = new char[size];
> @@ -157,7 +149,6 @@ TEST_F(CkptEncDecTest, testEncDecAvdComp
>   
> delete [] tmpData;
>   
> -  memset(&comp, '\0', sizeof (AVD_COMP));
> decode_comp(&enc.io_uba, &comp);
>   
> ASSERT_EQ(Amf::to_string(&comp.comp_info.name), "CompName");
> @@ -165,7 +156,7 @@ TEST_F(CkptEncDecTest, testEncDecAvdComp
> ASSERT_EQ(comp.saAmfCompReadinessState, 
> static_cast(0x55443322));
> ASSERT_EQ(comp.saAmfCompPresenceState, 
> static_cast(0x66554433));
> ASSERT_EQ(comp.saAmfCompRestartCount, static_cast(0x77665544));
> -  ASSERT_EQ(Amf::to_string(&comp.saAmfCompCurrProxyName), "CompProxyName");
> +  ASSERT_EQ(comp.saAmfCompCurrProxyName, "CompProxyName");
>   }
>   
>   TEST_F(CkptEncDecTest, testEncDecAvdSiAss) {
> @@ -182,15 +173,15 @@ TEST_F(CkptEncDecTest, testEncDecAvdSiAs
> rc = nc

[devel] Review Request for amf: update PR doc for long DN support [#1642]

[Long Nguyen]

Summary: amf: update PR doc for long DN support [#1642]
Review request for Trac Ticket(s): #1642
Peer Reviewer(s): AMF devs
Pull request to: AMF maintainers
Affected branch(es): default
Development branch: default


Impacted area   Impact y/n

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


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

amf: update PR doc for long DN support [#1642]

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.



OpenSAF_AMF_PR.odt
Description: application/vnd.oasis.opendocument.text
--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfa: fixed freeing notification buff [#1642]

[Long Nguyen]

Hi Praveen,

Please see my answers marked with [Long].

Best regards,
Long Nguyen.

On 8/19/2016 6:01 PM, praveen malviya wrote:
> Hi Long,
>
> I see one problem if B.01.01 application frees the memory in pg 
> tracking callback.
> Please see inline.
>
> Thanks,
> Praveen
> On 19-Aug-16 12:00 PM, Long HB Nguyen wrote:
>>  osaf/libs/agents/saf/amfa/amf_agent.cc | 1 +
>>  osaf/libs/agents/saf/amfa/ava_hdl.cc   |  2 --
>>  2 files changed, 1 insertions(+), 2 deletions(-)
>>
>>
>> diff --git a/osaf/libs/agents/saf/amfa/amf_agent.cc 
>> b/osaf/libs/agents/saf/amfa/amf_agent.cc
>> --- a/osaf/libs/agents/saf/amfa/amf_agent.cc
>> +++ b/osaf/libs/agents/saf/amfa/amf_agent.cc
>> @@ -2450,6 +2450,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTra
>>ava_cpy_protection_group_ntf(buf->notification, 
>> rsp_buf->notification,
>> buf->numberOfItems, 
>> SA_AMF_HARS_READY_FOR_ASSIGNMENT);
>>rc = SA_AIS_ERR_NO_SPACE;
>> +  buf->numberOfItems = rsp_buf->numberOfItems;
>>  }
>>} else {/* if(create_memory == false) */
>>
>> diff --git a/osaf/libs/agents/saf/amfa/ava_hdl.cc 
>> b/osaf/libs/agents/saf/amfa/ava_hdl.cc
>> --- a/osaf/libs/agents/saf/amfa/ava_hdl.cc
>> +++ b/osaf/libs/agents/saf/amfa/ava_hdl.cc
>> @@ -697,7 +697,6 @@ uint32_t ava_hdl_cbk_rec_prc(AVSV_AMF_CB
>> ((SaAmfCallbacksT_4*)reg_cbk)->saAmfProtectionGroupTrackCallback(&pg_track->csi_name,
>>  &buf,
>> pg_track->mem_num, pg_track->err);
>> -free(buf.notification);
>>  } else {
>>  pg_track->err = SA_AIS_ERR_NO_MEMORY;
>>  LOG_CR("Notification is NULL: Invoking 
>> PGTrack Callback with error SA_AIS_ERR_NO_MEMORY");
>> @@ -740,7 +739,6 @@ uint32_t ava_hdl_cbk_rec_prc(AVSV_AMF_CB
>>  ((SaAmfCallbacksT 
>> *)reg_cbk)->saAmfProtectionGroupTrackCallback(&pg_track->csi_name,
>> &buf,
>> pg_track->mem_num, pg_track->err);
>> -free(buf.notification);
> For B.04.01 API, saAmfProtectionGroupNotificationFree_4() is taking 
> care of freeing any extended name. For 
> saAmfProtectionGroupNotificationFree(), it is the application's 
> responsibility to free the memory. But how it will free any extended 
> name.
> I think there is no API equivalent to osaf_extended_name_free() for 
> application. Is there any way?
> [Long] I think we can do somethings like in applications:
> if (strlen(saAisNameBorrow(buff.notification[i].member.comp_name)) > 
> SA_MAX_UNEXTENDED_NAME_LENGTH)
> free(saAisNameBorrow(buff.notification[i].member.comp_name));
>
> Otherwise we can document it that:
> -if compName is not long dn in the notification buffer, then 
> application has to free the memory.This will provide backward 
> compatibility and spec compliance.
>
>  -if compName is longdn in notification then application should not 
> free the memory. Agent will free the memory after callback is 
> completed. So any B.01.01 application adapting to long dn will take 
> care of this when modifying the application. In that case we need to 
> do something like this :
> for i in notificationBuffer->notification[i]
>   if 
> (osaf_is_an_extended_name(notificationBuffer->notification[i].member.compName)){
> longdn_found = true;
> osaf_extended_name_free();
> }
> }
> if(longdn_found)
> free(buf.notification)
>
>
> Also there is a Todo in amf_agent.cc " // TODO (minhchau): memleak if 
> notification is an array".
> [Long] Thanks, I will check it.
>
> Thanks,
> Praveen
>
>>  } else {
>>  pg_track->err = SA_AIS_ERR_NO_MEMORY;
>>  LOG_CR("Notification is NULL: Invoking 
>> PGTrack Callback with error SA_AIS_ERR_NO_MEMORY");
>>
>


--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 5] amfd: replace SaNameT with string in include dir [#1642]

[Long Nguyen]

Hi Praveen,

Yes, the option 3 has a problem to make the names to be unique.
For the option 2, I agree that it will be complex.
We would like to understand more in the situation. Why do we need to 
support adding csi (long DN) dynamically to an application (does not 
support long DN) while we can add a normal (short) csi instead?
Can you please share with us the scenario?

Best regards,
Long Nguyen.

On 8/16/2016 4:19 PM, praveen malviya wrote:
>
>
> On 15-Aug-16 5:05 PM, Long Nguyen wrote:
>> Hi Praveen,
>>
>> Thanks for your suggestions.
>> Please see my comments marked with [Long[.
>>
>> Best regards,
>> Long Nguyen.
>>
>> On 8/12/2016 5:03 PM, praveen malviya wrote:
>>>
>>>
>>> On 12-Aug-16 1:27 PM, Long Nguyen wrote:
>>>> Hi Praveen,
>>>>
>>>> Actually, since Anders introduced the extended SaNameT in leap 
>>>> core, he
>>>> also added the osaf_extended_name_init() into leap.
>>>> Amf agent uses  leap library (i.e. saAmfInitialize()). So, 
>>>> applications
>>>> under amf control enabled long DN implicitly.
>>>> The SA_AIS_ERR_NAME_TOO_LONG return code is not returned in our case
>>>> (still SA_AIS_OK) due to the above reason. Consequently, the
>>>> applications are not crashed.
>>>>
>>>> I am not sure if it is right to add osaf_extended_name_init() into 
>>>> leap.
>>>> However, it is the case now.
>>>>
>>> Addition of osaf_extended_name_init()  into leap is right because
>>> every agent has to support long and short dn applications. But
>>> osaf_extended_name_init() distinguish a long or short dn application
>>> based on whether application has set "SA_ENABLE_EXTENDED_NAMES" or
>>> not.If you see the implementation of this API, based on this exported
>>> variable it remembers whether application is a long dn one or short dn
>>> one. For this only only applications are recommended via documentation
>>> to set this environment variable before calling any SAF API if they
>>> want handle longdn.In this way when first SAF API is called (generally
>>> sa,*>Initialize()), leap initialization will be done as a part of
>>> agent creation. Inside leap init it will call
>>> osaf_extended_name_init() which will use the environment variable to
>>> remember about the nature of application.
>>>
>>> The idea of CCB rejection was based on this only that an agent knows
>>> via osaf_extended_* APIs that it is created by a longdn or shortdn
>>> application. And this information can be passed to amfnd and hence to
>>> AMFD.
>>>
>>> But now what needs to be explored is why AMF Dispatch() is not
>>> returning NAME_TOO_LONG when you have already coded for it? Since old
>>> AMF demo has not enabled long dn, Dispatch() must return this error 
>>> code.
>>> Is there any problem with ava_sanamet_is_valid()?
>>> Let us debug that.
>>>
>>>
>>>> Can you please share with us your ideas? Do we still need to truncate
>>>> long names in this case?
>>> As we have discussed, rejection of CCB can be postponed as it requires
>>> new messages or introduction of new fields in old messages, all the
>>> way from AMFA to AMFD.
>>> So we are left with three options:
>>> 1) Dispatch() will return NAME_TOO_LONG without invoking callback.
>>> Current patch is doing this.
>>> This has one disadvantage that comp/application may exit/crash.
>>> 2)Dispatch will return SA_AIS_OK "without" invoking the callback.
>>> This will not confuse application.
>>> There is one disadvantage here, AMF has an illegal COMPCSI.
>>> 3)Dispatch will return SA_AIS_OK "by" invoking the callback with
>>> truncated values.
>>> This has one advantage that AMF does not have illegal COMPCSI.
>>> One disadvantage is that if comp is serving more CSI and then it
>>> will not be able to rightly distinguish between the CSI names.
>>>
>>> Option 2) sounds Ok to me as it will not create problems with
>>> application as AMF is not invoking any callback and atleast
>>> application is serving existing CSIs. Regarding the illegal COMPCSI in
>>> AMF: A user can always delete it and deletion will not again result in
>>> CSI remove callback because of the same reasons. At the same time we
>>> are cautioning the user via documentation (REAMDME and PR doc).
>>> [Long] I think the option 3 is bette

Re: [devel] [PATCH 1 of 5] amfd: replace SaNameT with string in include dir [#1642]

[Long Nguyen]

Hi Praveen,

Thanks for your suggestions.
Please see my comments marked with [Long[.

Best regards,
Long Nguyen.

On 8/12/2016 5:03 PM, praveen malviya wrote:
>
>
> On 12-Aug-16 1:27 PM, Long Nguyen wrote:
>> Hi Praveen,
>>
>> Actually, since Anders introduced the extended SaNameT in leap core, he
>> also added the osaf_extended_name_init() into leap.
>> Amf agent uses  leap library (i.e. saAmfInitialize()). So, applications
>> under amf control enabled long DN implicitly.
>> The SA_AIS_ERR_NAME_TOO_LONG return code is not returned in our case
>> (still SA_AIS_OK) due to the above reason. Consequently, the
>> applications are not crashed.
>>
>> I am not sure if it is right to add osaf_extended_name_init() into leap.
>> However, it is the case now.
>>
> Addition of osaf_extended_name_init()  into leap is right because 
> every agent has to support long and short dn applications. But 
> osaf_extended_name_init() distinguish a long or short dn application 
> based on whether application has set "SA_ENABLE_EXTENDED_NAMES" or 
> not.If you see the implementation of this API, based on this exported 
> variable it remembers whether application is a long dn one or short dn 
> one. For this only only applications are recommended via documentation 
> to set this environment variable before calling any SAF API if they 
> want handle longdn.In this way when first SAF API is called (generally 
> sa,*>Initialize()), leap initialization will be done as a part of 
> agent creation. Inside leap init it will call 
> osaf_extended_name_init() which will use the environment variable to 
> remember about the nature of application.
>
> The idea of CCB rejection was based on this only that an agent knows
> via osaf_extended_* APIs that it is created by a longdn or shortdn 
> application. And this information can be passed to amfnd and hence to 
> AMFD.
>
> But now what needs to be explored is why AMF Dispatch() is not 
> returning NAME_TOO_LONG when you have already coded for it? Since old 
> AMF demo has not enabled long dn, Dispatch() must return this error code.
> Is there any problem with ava_sanamet_is_valid()?
> Let us debug that.
>
>
>> Can you please share with us your ideas? Do we still need to truncate
>> long names in this case?
> As we have discussed, rejection of CCB can be postponed as it requires 
> new messages or introduction of new fields in old messages, all the 
> way from AMFA to AMFD.
> So we are left with three options:
> 1) Dispatch() will return NAME_TOO_LONG without invoking callback. 
> Current patch is doing this.
> This has one disadvantage that comp/application may exit/crash.
> 2)Dispatch will return SA_AIS_OK "without" invoking the callback.
> This will not confuse application.
> There is one disadvantage here, AMF has an illegal COMPCSI.
> 3)Dispatch will return SA_AIS_OK "by" invoking the callback with 
> truncated values.
> This has one advantage that AMF does not have illegal COMPCSI.
> One disadvantage is that if comp is serving more CSI and then it 
> will not be able to rightly distinguish between the CSI names.
>
> Option 2) sounds Ok to me as it will not create problems with 
> application as AMF is not invoking any callback and atleast 
> application is serving existing CSIs. Regarding the illegal COMPCSI in 
> AMF: A user can always delete it and deletion will not again result in 
> CSI remove callback because of the same reasons. At the same time we 
> are cautioning the user via documentation (REAMDME and PR doc).
> [Long] I think the option 3 is better than the option 2. If we ignore 
> the callback, amf does not receive the responses from applications and 
> then applications get restarted due to callback timeout.
>
> thanks,
> Praveen
>
>
>>
>> Best regards,
>> Long Nguyen.
>>
>> On 8/11/2016 3:04 PM, Long Nguyen wrote:
>>> Hi Praveen,
>>>
>>> Thanks for your suggestion.
>>> In the situation you described below, you add a csi dynamically (long
>>> DN) to an application (not support long DN).
>>> So, we only need to truncate the csi name. This will help the
>>> application not crashed.
>>> In my opinion, we only need to truncate in case of CSISet or
>>> CSIRemove. There may be some problems with applications associated
>>> with more than one csi, if the applications base on csi names to do
>>> special things.
>>> In case of protection group, when we call saAmfProtectionGroupTrack,
>>> we have to specify the csi_name. It is not in the case of long DN
>>> since the application does not support long DN.
>>>
&

Re: [devel] [PATCH 1 of 5] amfd: replace SaNameT with string in include dir [#1642]

[Long Nguyen]

Hi Praveen,

Thanks so much. I am checking and fixing.

Best regards,
Long Nguyen.

On 8/12/2016 6:40 PM, praveen malviya wrote:
> Hi Long,
>
> I have checked. The problem is not with ava_sanamet_is_valid(). Agent 
> is getting that env variable for AMFND when it instantiates the 
> component using CLC-CLI script. This needs to be fixed in amfnd patch.
>
> Thanks,
> Praveen
>
>
>
> On 12-Aug-16 3:33 PM, praveen malviya wrote:
>>
>>
>> On 12-Aug-16 1:27 PM, Long Nguyen wrote:
>>> Hi Praveen,
>>>
>>> Actually, since Anders introduced the extended SaNameT in leap core, he
>>> also added the osaf_extended_name_init() into leap.
>>> Amf agent uses  leap library (i.e. saAmfInitialize()). So, applications
>>> under amf control enabled long DN implicitly.
>>> The SA_AIS_ERR_NAME_TOO_LONG return code is not returned in our case
>>> (still SA_AIS_OK) due to the above reason. Consequently, the
>>> applications are not crashed.
>>>
>>> I am not sure if it is right to add osaf_extended_name_init() into 
>>> leap.
>>> However, it is the case now.
>>>
>> Addition of osaf_extended_name_init()  into leap is right because every
>> agent has to support long and short dn applications. But
>> osaf_extended_name_init() distinguish a long or short dn application
>> based on whether application has set "SA_ENABLE_EXTENDED_NAMES" or
>> not.If you see the implementation of this API, based on this exported
>> variable it remembers whether application is a long dn one or short dn
>> one. For this only only applications are recommended via documentation
>> to set this environment variable before calling any SAF API if they want
>> handle longdn.In this way when first SAF API is called (generally
>> sa,*>Initialize()), leap initialization will be done as a part of agent
>> creation. Inside leap init it will call osaf_extended_name_init() which
>> will use the environment variable to remember about the nature of
>> application.
>>
>> The idea of CCB rejection was based on this only that an agent knows
>> via osaf_extended_* APIs that it is created by a longdn or shortdn
>> application. And this information can be passed to amfnd and hence to 
>> AMFD.
>>
>> But now what needs to be explored is why AMF Dispatch() is not returning
>> NAME_TOO_LONG when you have already coded for it? Since old AMF demo has
>> not enabled long dn, Dispatch() must return this error code.
>> Is there any problem with ava_sanamet_is_valid()?
>> Let us debug that.
>>
>>
>>> Can you please share with us your ideas? Do we still need to truncate
>>> long names in this case?
>> As we have discussed, rejection of CCB can be postponed as it requires
>> new messages or introduction of new fields in old messages, all the way
>> from AMFA to AMFD.
>> So we are left with three options:
>> 1) Dispatch() will return NAME_TOO_LONG without invoking callback.
>> Current patch is doing this.
>> This has one disadvantage that comp/application may exit/crash.
>> 2)Dispatch will return SA_AIS_OK "without" invoking the callback.
>> This will not confuse application.
>> There is one disadvantage here, AMF has an illegal COMPCSI.
>> 3)Dispatch will return SA_AIS_OK "by" invoking the callback with
>> truncated values.
>> This has one advantage that AMF does not have illegal COMPCSI.
>> One disadvantage is that if comp is serving more CSI and then it 
>> will
>> not be able to rightly distinguish between the CSI names.
>>
>> Option 2) sounds Ok to me as it will not create problems with
>> application as AMF is not invoking any callback and atleast application
>> is serving existing CSIs. Regarding the illegal COMPCSI in AMF: A user
>> can always delete it and deletion will not again result in CSI remove
>> callback because of the same reasons. At the same time we are cautioning
>> the user via documentation (REAMDME and PR doc).
>>
>>
>> thanks,
>> Praveen
>>
>>
>>>
>>> Best regards,
>>> Long Nguyen.
>>>
>>> On 8/11/2016 3:04 PM, Long Nguyen wrote:
>>>> Hi Praveen,
>>>>
>>>> Thanks for your suggestion.
>>>> In the situation you described below, you add a csi dynamically (long
>>>> DN) to an application (not support long DN).
>>>> So, we only need to truncate the csi name. This will help the
>>>> application not crashed.
>>>> In my opinion, we only need to truncate in case 

Re: [devel] [PATCH 1 of 5] amfd: replace SaNameT with string in include dir [#1642]

[Long Nguyen]

Hi Praveen,

Actually, since Anders introduced the extended SaNameT in leap core, he 
also added the osaf_extended_name_init() into leap.
Amf agent uses  leap library (i.e. saAmfInitialize()). So, applications 
under amf control enabled long DN implicitly.
The SA_AIS_ERR_NAME_TOO_LONG return code is not returned in our case 
(still SA_AIS_OK) due to the above reason. Consequently, the 
applications are not crashed.

I am not sure if it is right to add osaf_extended_name_init() into leap. 
However, it is the case now.

Can you please share with us your ideas? Do we still need to truncate 
long names in this case?

Best regards,
Long Nguyen.

On 8/11/2016 3:04 PM, Long Nguyen wrote:
> Hi Praveen,
>
> Thanks for your suggestion.
> In the situation you described below, you add a csi dynamically (long 
> DN) to an application (not support long DN).
> So, we only need to truncate the csi name. This will help the 
> application not crashed.
> In my opinion, we only need to truncate in case of CSISet or 
> CSIRemove. There may be some problems with applications associated 
> with more than one csi, if the applications base on csi names to do 
> special things.
> In case of protection group, when we call saAmfProtectionGroupTrack, 
> we have to specify the csi_name. It is not in the case of long DN 
> since the application does not support long DN.
>
> Best regards,
> Long Nguyen.
>
> On 8/11/2016 1:29 PM, praveen malviya wrote:
>> Hi,
>>
>> I think, as of now, approaches discussed in this mail thread can be 
>> mixed for a temporary solution: Dispatch() can return SA_AIS_OK by 
>> invoking the callback(s) with truncated values for any long field 
>> (like comp name, csi name etc). In this way comp will have callbacks 
>> and AMF will also be maintaining legal COMPCSIs. Actual values can be 
>> logged. Also the the PR doc and Readme should talk how truncation 
>> will be performed.
>> I think, this can be done in this release.
>> Other solution of rejecting the CCB itself can be postponed as of now.
>>
>>
>> Thanks,
>> Praveen
>>
>> On 11-Aug-16 10:03 AM, Long Nguyen wrote:
>>> Hi Praveen and others,
>>>
>>> I have an idea marked with [Long] below.
>>>
>>> Best regards,
>>> Long Nguyen.
>>>
>>> On 8/3/2016 1:45 PM, praveen malviya wrote:
>>>>
>>>>
>>>> On 02-Aug-16 3:39 AM, minh chau wrote:
>>>>> Hi Praveen,
>>>>>
>>>>> One comment with [Minh] in line.
>>>>>
>>>>> Thanks,
>>>>> Minh
>>>>>
>>>>> On 01/08/16 17:22, Gary Lee wrote:
>>>>>> Hi Praveen
>>>>>>
>>>>>> On 1/08/2016 4:29 PM, praveen malviya wrote:
>>>>>>> Hi Gary, Long,
>>>>>>>
>>>>>>> Some comments/observations:
>>>>>>> -In AMFD saAisNameBorrow() is used in logging and AMFND uses
>>>>>>> osaf_extended_name_borrow().
>>>>>>> For osaf_extended_name_borrow() note in osaf_extended_name.h 
>>>>>>> says it
>>>>>>> is intended for mainly agent libraries. But middle-ware services
>>>>>>> always use core libs. At the same time saAisNameBorrow(), I 
>>>>>>> think, is
>>>>>>> for application.
>>>>>>> any reason of using them this way and what is the recommended way?
>>>>>> I think I used both styles in amfd. I think we can change 
>>>>>> saAisNameXX
>>>>>> to osaf_extended_name_XX just before pushing, to make it consistent
>>>>>> with the rest of the OpenSAF services.
>>>>>>> -I think, one case may arrive from upgrade perspective.
>>>>>>> Suppose any application (say amf_demo app) is running without
>>>>>>> enabling long dn and a csi, with its RDn greater than 256, is added
>>>>>>> dynamically (long dn enabled in IMM). In this case AMFD will assign
>>>>>>> this csi to the running component. Component will not be able to 
>>>>>>> read
>>>>>>> the CSI and may crash.
>>>>>>> This is related to invocation of CSI_SET callback but same will be
>>>>>>> valid for PG tracking also. There may be other cases also.
>>>>>>> Even truncation will not work in this case.
>>>>> [Minh] I think the agent patch that Gary submitted currently returns
>>>>> SA_AIS_ERR_NAME_TOO_LONG in saAmfDispatch() 

Re: [devel] [PATCH 1 of 5] amfd: replace SaNameT with string in include dir [#1642]

[Long Nguyen]

Hi Praveen,

Thanks for your suggestion.
In the situation you described below, you add a csi dynamically (long 
DN) to an application (not support long DN).
So, we only need to truncate the csi name. This will help the 
application not crashed.
In my opinion, we only need to truncate in case of CSISet or CSIRemove. 
There may be some problems with applications associated with more than 
one csi, if the applications base on csi names to do special things.
In case of protection group, when we call saAmfProtectionGroupTrack, we 
have to specify the csi_name. It is not in the case of long DN since the 
application does not support long DN.

Best regards,
Long Nguyen.

On 8/11/2016 1:29 PM, praveen malviya wrote:
> Hi,
>
> I think, as of now, approaches discussed in this mail thread can be 
> mixed for a temporary solution: Dispatch() can return SA_AIS_OK by 
> invoking the callback(s) with truncated values for any long field 
> (like comp name, csi name etc). In this way comp will have callbacks 
> and AMF will also be maintaining legal COMPCSIs. Actual values can be 
> logged. Also the the PR doc and Readme should talk how truncation will 
> be performed.
> I think, this can be done in this release.
> Other solution of rejecting the CCB itself can be postponed as of now.
>
>
> Thanks,
> Praveen
>
> On 11-Aug-16 10:03 AM, Long Nguyen wrote:
>> Hi Praveen and others,
>>
>> I have an idea marked with [Long] below.
>>
>> Best regards,
>> Long Nguyen.
>>
>> On 8/3/2016 1:45 PM, praveen malviya wrote:
>>>
>>>
>>> On 02-Aug-16 3:39 AM, minh chau wrote:
>>>> Hi Praveen,
>>>>
>>>> One comment with [Minh] in line.
>>>>
>>>> Thanks,
>>>> Minh
>>>>
>>>> On 01/08/16 17:22, Gary Lee wrote:
>>>>> Hi Praveen
>>>>>
>>>>> On 1/08/2016 4:29 PM, praveen malviya wrote:
>>>>>> Hi Gary, Long,
>>>>>>
>>>>>> Some comments/observations:
>>>>>> -In AMFD saAisNameBorrow() is used in logging and AMFND uses
>>>>>> osaf_extended_name_borrow().
>>>>>> For osaf_extended_name_borrow() note in osaf_extended_name.h says it
>>>>>> is intended for mainly agent libraries. But middle-ware services
>>>>>> always use core libs. At the same time saAisNameBorrow(), I 
>>>>>> think, is
>>>>>> for application.
>>>>>> any reason of using them this way and what is the recommended way?
>>>>> I think I used both styles in amfd. I think we can change saAisNameXX
>>>>> to osaf_extended_name_XX just before pushing, to make it consistent
>>>>> with the rest of the OpenSAF services.
>>>>>> -I think, one case may arrive from upgrade perspective.
>>>>>> Suppose any application (say amf_demo app) is running without
>>>>>> enabling long dn and a csi, with its RDn greater than 256, is added
>>>>>> dynamically (long dn enabled in IMM). In this case AMFD will assign
>>>>>> this csi to the running component. Component will not be able to 
>>>>>> read
>>>>>> the CSI and may crash.
>>>>>> This is related to invocation of CSI_SET callback but same will be
>>>>>> valid for PG tracking also. There may be other cases also.
>>>>>> Even truncation will not work in this case.
>>>> [Minh] I think the agent patch that Gary submitted currently returns
>>>> SA_AIS_ERR_NAME_TOO_LONG in saAmfDispatch() if long DN callback 
>>>> comes to
>>>> legacy application (unadapted long DN app). The real callback won't be
>>>> issued but application may crash if it exit() on non-SA_AIS_OK from
>>>> Dispatch(). I guess you have seen this with #1553? Do you think it's
>>>> good way if amf agent drops the long DN callback and also Dispatch()
>>>> returns OK to legacy app, and print error in syslog?
>>> Not observed in the context of #1553, but I remember about such a fix
>>> in NTF long dn changes.
>>> Returning SA_AIS_OK in Dispatch() call saves application from crash,
>>> but the problem in AMF is still different as it will maintain a
>>> COMPCSI relationship without comp being actually assigned for that.
>>>
>>> Can we think of a way where AMF will not allow this conf change by
>>> rejecting the CCB operation itself. For this AMF should know that this
>>> application supports Long dn or not. But this information needs to be
>>&g

Re: [devel] [PATCH 1 of 5] amfd: replace SaNameT with string in include dir [#1642]

[Long Nguyen]

Hi Praveen and others,

I have an idea marked with [Long] below.

Best regards,
Long Nguyen.

On 8/3/2016 1:45 PM, praveen malviya wrote:
>
>
> On 02-Aug-16 3:39 AM, minh chau wrote:
>> Hi Praveen,
>>
>> One comment with [Minh] in line.
>>
>> Thanks,
>> Minh
>>
>> On 01/08/16 17:22, Gary Lee wrote:
>>> Hi Praveen
>>>
>>> On 1/08/2016 4:29 PM, praveen malviya wrote:
>>>> Hi Gary, Long,
>>>>
>>>> Some comments/observations:
>>>> -In AMFD saAisNameBorrow() is used in logging and AMFND uses
>>>> osaf_extended_name_borrow().
>>>> For osaf_extended_name_borrow() note in osaf_extended_name.h says it
>>>> is intended for mainly agent libraries. But middle-ware services
>>>> always use core libs. At the same time saAisNameBorrow(), I think, is
>>>> for application.
>>>> any reason of using them this way and what is the recommended way?
>>> I think I used both styles in amfd. I think we can change saAisNameXX
>>> to osaf_extended_name_XX just before pushing, to make it consistent
>>> with the rest of the OpenSAF services.
>>>> -I think, one case may arrive from upgrade perspective.
>>>> Suppose any application (say amf_demo app) is running without
>>>> enabling long dn and a csi, with its RDn greater than 256, is added
>>>> dynamically (long dn enabled in IMM). In this case AMFD will assign
>>>> this csi to the running component. Component will not be able to read
>>>> the CSI and may crash.
>>>> This is related to invocation of CSI_SET callback but same will be
>>>> valid for PG tracking also. There may be other cases also.
>>>> Even truncation will not work in this case.
>> [Minh] I think the agent patch that Gary submitted currently returns
>> SA_AIS_ERR_NAME_TOO_LONG in saAmfDispatch() if long DN callback comes to
>> legacy application (unadapted long DN app). The real callback won't be
>> issued but application may crash if it exit() on non-SA_AIS_OK from
>> Dispatch(). I guess you have seen this with #1553? Do you think it's
>> good way if amf agent drops the long DN callback and also Dispatch()
>> returns OK to legacy app, and print error in syslog?
> Not observed in the context of #1553, but I remember about such a fix 
> in NTF long dn changes.
> Returning SA_AIS_OK in Dispatch() call saves application from crash, 
> but the problem in AMF is still different as it will maintain a 
> COMPCSI relationship without comp being actually assigned for that.
>
> Can we think of a way where AMF will not allow this conf change by 
> rejecting the CCB operation itself. For this AMF should know that this 
> application supports Long dn or not. But this information needs to be 
> carried from agent to AMFD.
> Before going for such a way, I think, we can ask opinion from other 
> AMF maintainers.
> [Long] You have the good solution. However, it is hard to know if an 
> application supports long DN or not at the time of CCB operration. Can 
> we make your suggestion as an enhancement later?
>
> Thanks,
> Praveen
>
>
>
>>>> - While running some tests observed crashes in amfnd and amfd.
>>>> I will update #1642 with bt information.
>>> Minh will answer this bit.
>>>
>>> Thanks
>>> Gary
>>>
>>
>


--
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] amfa: enable long DN support [#1642]

[Long Nguyen]

Hi Praveen,

Thanks for your comments.
I will update the patch and send the V2 version.

Best regards,
Long Nguyen.

On 8/4/2016 4:55 PM, praveen malviya wrote:
> Please find some comments inline with [Praveen]
>
>
> Thanks,
> Praveen
>
> On 11-Jul-16 3:38 PM, Long HB Nguyen wrote:
>>  osaf/libs/agents/saf/amfa/Makefile.am |1 +
>>  osaf/libs/agents/saf/amfa/amf_agent.cc  |  638 
>> ++-
>>  osaf/libs/agents/saf/amfa/ava_hdl.cc|  246 +
>>  osaf/libs/agents/saf/amfa/ava_init.cc   |   21 +-
>>  osaf/libs/agents/saf/amfa/ava_mds.cc|  448 +++-
>>  osaf/libs/agents/saf/amfa/ava_op.cc |   23 +-
>>  osaf/libs/agents/saf/amfa/include/ava.h |1 +
>>  osaf/libs/agents/saf/amfa/include/ava_cb.h  |1 +
>>  osaf/libs/agents/saf/amfa/include/ava_mds.h |  267 +-
>>  9 files changed, 979 insertions(+), 667 deletions(-)
>>
>>
>> diff --git a/osaf/libs/agents/saf/amfa/Makefile.am 
>> b/osaf/libs/agents/saf/amfa/Makefile.am
>> --- a/osaf/libs/agents/saf/amfa/Makefile.am
>> +++ b/osaf/libs/agents/saf/amfa/Makefile.am
>> @@ -25,6 +25,7 @@ noinst_LTLIBRARIES = libava.la
>>  libava_la_CXXFLAGS = $(AM_CXXFLAGS)
>>
>>  libava_la_CPPFLAGS = \
>> +-DSA_EXTENDED_NAME_SOURCE \
>>  $(AM_CPPFLAGS) \
>>  -I$(top_srcdir)/osaf/libs/common/amf/include \
>>  -I$(top_srcdir)/osaf/libs/agents/saf/amfa/include
>> diff --git a/osaf/libs/agents/saf/amfa/amf_agent.cc 
>> b/osaf/libs/agents/saf/amfa/amf_agent.cc
>> --- a/osaf/libs/agents/saf/amfa/amf_agent.cc
>> +++ b/osaf/libs/agents/saf/amfa/amf_agent.cc
>> @@ -366,7 +366,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHand
>>}
>>
>>/* populate & send the finalize message */
>> -  m_AVA_AMF_FINALIZE_MSG_FILL(msg, cb->ava_dest, hdl, cb->comp_name);
>> +  ava_fill_finalize_msg(&msg, cb->ava_dest, hdl, cb->comp_name);
>>rc = static_cast(ava_mds_send(cb, &msg, 0));
>>if (NCSCC_RC_SUCCESS == rc) {
>>  ncshm_give_hdl(hdl);
>> @@ -429,13 +429,12 @@ SaAisErrorT AmfAgent::ComponentRegister(
>>AVA_HDL_REC *hdl_rec = 0;
>>AVSV_NDA_AVA_MSG msg = {};
>>AVSV_NDA_AVA_MSG *msg_rsp = 0;
>> -  SaNameT pcomp_name = {0};
>> +  SaNameT pcomp_name;
>>SaAisErrorT rc = SA_AIS_OK;
>>TRACE_ENTER2("SaAmfHandleT passed is %llx", hdl);
>>
>> -  if (!comp_name || !(comp_name->length) ||
>> -  (comp_name->length > SA_MAX_NAME_LENGTH) ||
>> -  (proxy_comp_name && (!proxy_comp_name->length || 
>> proxy_comp_name->length > SA_MAX_NAME_LENGTH))) {
>> +  if (!comp_name || !ava_sanamet_is_valid(comp_name) ||
>> +(proxy_comp_name && !ava_sanamet_is_valid(proxy_comp_name))) {
>>  TRACE_LEAVE2("Incorrect arguments");
>>  return SA_AIS_ERR_INVALID_PARAM;
>>}
>> @@ -461,15 +460,17 @@ SaAisErrorT AmfAgent::ComponentRegister(
>>}
>>
>>if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
>> -if (getenv("SA_AMF_COMPONENT_NAME")) {
>> -  if (strlen(getenv("SA_AMF_COMPONENT_NAME")) < 
>> SA_MAX_NAME_LENGTH) {
>> -strcpy((char *)cb->comp_name.value, 
>> getenv("SA_AMF_COMPONENT_NAME"));
>> -cb->comp_name.length = (uint16_t)strlen((char 
>> *)cb->comp_name.value);
>> -m_AVA_FLAG_SET(cb, AVA_FLAG_COMP_NAME);
>> -  } else {
>> -TRACE_2("Length of SA_AMF_COMPONENT_NAME exceeds 
>> SA_MAX_NAME_LENGTH bytes");
>> +SaConstStringT env_comp_name = getenv("SA_AMF_COMPONENT_NAME");
>> +if (env_comp_name) {
>> +  if (strlen(env_comp_name) > kOsafMaxDnLength) {
>> +TRACE_2("Length of SA_AMF_COMPONENT_NAME exceeds "
>> +"kOsafMaxDnLength(%d) bytes", kOsafMaxDnLength);
>>  rc = SA_AIS_ERR_INVALID_PARAM;
>>  goto done;
>> +  } else {
>> +// @cb->comp_name could be longDN, need to be freed later
>> +osaf_extended_name_alloc(env_comp_name, &cb->comp_name);
>> +m_AVA_FLAG_SET(cb, AVA_FLAG_COMP_NAME);
>>}
>>  } else {
>>TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is 
>> NULL");
>> @@ -481,26 +482,31 @@ SaAisErrorT AmfAgent::ComponentRegister(
>>  ncshm_give_hdl(gl_ava_hdl);
>>
>>/* non-proxied, component Length part of component name should be 
>> OK */
>> -  if (!proxy_comp_name && (comp_na

Re: [devel] [PATCH 00 of 30] Review Request for amfnd: convert NULL to nullptr [#1551]

[Long Nguyen]

Hi Hans,

Can you please also push the code for AMFND? Thanks so much.

Best regards,
Long Nguyen.

On 10/27/2015 7:09 PM, Hans Nordebäck wrote:

Ack for the series, code review only/Thanks HansN

-Original Message-
From: Long HB Nguyen [mailto:long.hb.ngu...@dektech.com.au]
Sent: den 20 oktober 2015 11:58
To: Hans Nordebäck; Gary Lee; praveen.malv...@oracle.com; nagendr...@oracle.com
Cc: opensaf-devel@lists.sourceforge.net
Subject: [PATCH 00 of 30] Review Request for amfnd: convert NULL to nullptr 
[#1551]

Summary: amfnd: convert NULL to nullptr [#1551] Review request for Trac 
Ticket(s): [#1551] Peer Reviewer(s): Hans, Gary, Praveen, Nagu Pull request to: 
Hans Affected branch(es): default Development branch: default


Impacted area   Impact y/n

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


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

changeset d9e5204b9cd64449385835be326c4635993233ca
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:08:29 +0700

amfnd: convert NULL to nullptr for amfnd.cc [#1551]

changeset 4aa0c0e69ea683b655e5ce95a48bd46668bb1947
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:13:52 +0700

amfnd: convert NULL to nullptr for cbq.cc [#1551]

changeset 8565a4b0f13090e2a248d4dc030107fe872c46e3
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:16:24 +0700

amfnd: convert NULL to nullptr for chc.cc [#1551]

changeset 61e799adaa100457d5da11741daab89abb312342
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:20:01 +0700

amfnd: convert NULL to nullptr for ckpt_dec.cc [#1551]

changeset 6146e7b22e233213be199892f03f8f2b87e7733a
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:22:03 +0700

amfnd: convert NULL to nullptr for ckpt_edu.cc [#1551]

changeset 3010ccff64a8b80f0b0ba2abeee3c3f569c06a36
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:23:20 +0700

amfnd: convert NULL to nullptr for ckpt_enc.cc [#1551]

changeset 19fff7f26c302b0a252c537979c5be117a96b8ab
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:24:52 +0700

amfnd: convert NULL to nullptr for ckpt_updt.cc [#1551]

changeset 58263076e522c0494bd91480001a68369818b0c9
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:26:02 +0700

amfnd: convert NULL to nullptr for clc.cc [#1551]

changeset ec0d86a8fcac950de12157c90406e1c60a688cc0
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:27:11 +0700

amfnd: convert NULL to nullptr for clm.cc [#1551]

changeset 6f24ab6b30da4a901bff37e3c65f628e478505d9
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:28:06 +0700

amfnd: convert NULL to nullptr for comp.cc [#1551]

changeset 114de32db084f1987a80fa450509619455702054
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:29:43 +0700

amfnd: convert NULL to nullptr for compdb.cc [#1551]

changeset 1351acca1f39d71e0d084a7bdb156d8168b73b16
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:42:44 +0700

amfnd: convert NULL to nullptr for cpm.cc [#1551]

changeset e850274601d9d382839c2538381c98b8151f08a2
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:43:26 +0700

amfnd: convert NULL to nullptr for di.cc [#1551]

changeset 6f75b3025f9d26fee62aa3e1e92750e86a9e596c
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:45:17 +0700

amfnd: convert NULL to nullptr for err.cc [#1551]

changeset 92a13233ca950df369ca41ab9d5241c815fe3bc3
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:46:49 +0700

amfnd: convert NULL to nullptr for evt.cc [#1551]

changeset 913ca62cb17743d241dda8672bcfc22773c3aae3
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:47:43 +0700

amfnd: convert NULL to nullptr for hcdb.cc [#1551]

changeset d18406ea8012bd153f83550b3a1c893996fe2d54
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:48:39 +0700

amfnd: convert NULL to nullptr for main.cc [#1551]

changeset 228c017ee533cc9a3d8aae0badc7a748b7f42584
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:50:13 +0700

amfnd: convert NULL to nullptr for mbcsv.cc [#1551]

changeset d6d3d5b4aa2a9c4ac41d9cfb6953ac2c6428200e
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:51:32 +0700

amfnd: convert NULL to nullptr for mds.cc [#1551]

changeset d1bf0e5468fd8908aa50273cb0b468d27f756ee7
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:52:13 +0700

amfnd: convert NULL to nullptr for mon.cc [#1551]

changeset c1dd617127604017f920b54bb2f3cd031ea66887
Author: Long HB Nguyen
Date:   Tue, 20 Oct 2015 14:53:13 +0700

amfnd: convert NULL to nullptr for pg.cc [#1551]

changeset 873947fc8c5a5de04ef0e8725428f13d7a2e0be1
Author: Long

Re: [devel] [PATCH 02 of 48] amfd: convert NULL to nullptr for apptype.cc [#1547]

[Long Nguyen]

Hi Praveen,

Thanks so much. I will change these #define in another ticket 
(https://sourceforge.net/p/opensaf/tickets/1558/).

Best regards,
Long Nguyen.

On 10/22/2015 5:57 PM, praveen malviya wrote:
> Ack for the series.
>
> There are still some #defines like :
>  AVD_SI_NULL,  AVD_SU_SI_REL_NULL, AVD_EVT_NULL and AVD_AVND_NULL.
> and they are used for NULL in AMDS code.
>
> Thanks,
> Praveen
>
> On 19-Oct-15 4:48 PM, Long HB Nguyen wrote:
>>   osaf/services/saf/amf/amfd/apptype.cc | 52 
>> +-
>>   1 files changed, 26 insertions(+), 26 deletions(-)
>>
>>
>> diff --git a/osaf/services/saf/amf/amfd/apptype.cc 
>> b/osaf/services/saf/amf/amfd/apptype.cc
>> --- a/osaf/services/saf/amf/amfd/apptype.cc
>> +++ b/osaf/services/saf/amf/amfd/apptype.cc
>> @@ -47,13 +47,13 @@ static void apptype_delete(AVD_APP_TYPE
>>
>>   (*apptype)->sgAmfApptSGTypes.clear();
>>   delete *apptype;
>> -*apptype = NULL;
>> +*apptype = nullptr;
>>   }
>>
>>   static void apptype_add_to_model(AVD_APP_TYPE *app_type)
>>   {
>>   unsigned int rc;
>> -osafassert(app_type != NULL);
>> +osafassert(app_type != nullptr);
>>   TRACE("'%s'", app_type->name.value);
>>
>>   rc = app_type_db->insert(Amf::to_string(&app_type->name), 
>> app_type);
>> @@ -69,7 +69,7 @@ static int is_config_valid(const SaNameT
>>   AVD_AMF_SG_TYPE *sg_type;
>>   const SaImmAttrValuesT_2 *attr;
>>
>> -if ((parent = strchr((char*)dn->value, ',')) == NULL) {
>> +if ((parent = strchr((char*)dn->value, ',')) == nullptr) {
>>   report_ccb_validation_error(opdata, "No parent to '%s' ", 
>> dn->value);
>>   return 0;
>>   }
>> @@ -80,7 +80,7 @@ static int is_config_valid(const SaNameT
>>   return 0;
>>   }
>>
>> -while ((attr = attributes[i++]) != NULL)
>> +while ((attr = attributes[i++]) != nullptr)
>>   if (!strcmp(attr->attrName, "saAmfApptSGTypes"))
>>   break;
>>
>> @@ -90,14 +90,14 @@ static int is_config_valid(const SaNameT
>>   for (j = 0; j < attr->attrValuesNumber; j++) {
>>   SaNameT *name = (SaNameT *)attr->attrValues[j];
>>   sg_type = sgtype_db->find(Amf::to_string(name));
>> -if (sg_type == NULL) {
>> -if (opdata == NULL) {
>> +if (sg_type == nullptr) {
>> +if (opdata == nullptr) {
>>   report_ccb_validation_error(opdata, "'%s' does not 
>> exist in model", name->value);
>>   return 0;
>>   }
>>
>>   /* SG type does not exist in current model, check CCB */
>> -if (ccbutil_getCcbOpDataByDN(opdata->ccbId, name) == 
>> NULL) {
>> +if (ccbutil_getCcbOpDataByDN(opdata->ccbId, name) == 
>> nullptr) {
>>   report_ccb_validation_error(opdata, "'%s' does not 
>> exist either in model or CCB",
>>   name->value);
>>   return 0;
>> @@ -120,7 +120,7 @@ static AVD_APP_TYPE *apptype_create(SaNa
>>
>>   app_type = new AVD_APP_TYPE(dn);
>>
>> -while ((attr = attributes[i++]) != NULL)
>> +while ((attr = attributes[i++]) != nullptr)
>>   if (!strcmp(attr->attrName, "saAmfApptSGTypes"))
>>   break;
>>
>> @@ -161,14 +161,14 @@ static SaAisErrorT apptype_ccb_completed
>>   break;
>>   case CCBUTIL_DELETE:
>>   app_type = avd_apptype_get(&opdata->objectName);
>> -if (NULL != app_type->list_of_app) {
>> +if (nullptr != app_type->list_of_app) {
>>   /* check whether there exists a delete operation for
>>* each of the App in the app_type list in the current CCB
>>*/
>>   app = app_type->list_of_app;
>> -while (app != NULL) {
>> +while (app != nullptr) {
>>   t_opData = ccbutil_getCcbOpDataByDN(opdata->ccbId, 
>> &app->name);
>> -if ((t_opData == NULL) || (t_opData->operationType 
>> != CCBUTIL_DELETE)) {
>> +if ((t_opData == nullptr) || 
>> (t_opData->operationType != CCBUTIL_DELETE)) {
>>   app_exist = true;
>>   br