date:20180122

Re: [devel] [PATCH 0/5] Review Request for smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

Hi

I have now sent a patch that concludes this series of increments for creating 
the new CCB handler.
I have created a test program (ccbhdl_test) that test many aspects of the 
handler including all possible modification types that can be added to a CCB, 
all attribute types that can be used with create and modify. It also test usage 
of a modification handler object in different scopes including using it several 
times without going out of scope.
I have used Valgrind with this test program to test for memory leeks, important 
since a lot of memory allocation is done in the CCB handler.

I have not yet tested very much recovery handling. To do this the code has to 
be "instrumented" with extra test code that simulates IMM error return codes, 
finalize handles to test BAD HANDLE etc. I will do some more of that kind of 
testing.

Next step is to concatenate all the increments into one commit and send it for 
an "official" review. When this is acked I plan to push without changing 
anything in SMF.
After that SMF can be refactored using this handler instead of inline spot 
fixes all over the place.

Thanks
Lennart


> -Original Message-
> From: Syam Prasad Talluri [mailto:syam.tall...@oracle.com]
> Sent: den 18 januari 2018 09:30
> To: Lennart Lund ; Vijay Roy
> 
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: RE: [devel] [PATCH 0/5] Review Request for smf: Add capability to
> redo CCBs that fail [#1398]
> 
> Hi Lennart,
> 
>   I understand you are planning to send some more patches. Here are my
> initial observations
> 
>   1. In om_handle.cc we are reading the env variable
> IMMOMCPP_TRACE_PATHNAME, we need to add about this in the ReadMe
> file or remove this env
>   2. Like immccb operations do we need to implement wrappers for
> saImmOmAccessorGet_2, saImmOmAdminOperationInvoke_2 which are
> used by SMF.
>   3. Wrappers are added for IMM OM, do we need to have same kind of
> wrapper for IMM OI implementation as well ?
>   4. In om_handle.h is there any special reason to keep the below functions
>   void SetDummy(int i) { dummy_ = 1; }
>   int GetDummy(void) { return dummy_; }
>   5. There are TODO's in some files, I hope you will address them before final
> commit.
>   6. Looks like Wrappers implemented are generic Hence we can use these in
> other services also and these can be moved to a common place going
> forward.
> 
>   We will try to integrate the existing SMF OM Interface with these Wrappers
> .
> 
> 
> Thanks,
> Syam.
> -Original Message-
> From: Lennart Lund [mailto:lennart.l...@ericsson.com]
> Sent: Tuesday, January 16, 2018 8:11 PM
> To: Vijay Roy 
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [devel] [PATCH 0/5] Review Request for smf: Add capability to redo
> CCBs that fail [#1398]
> 
> Summary: smf: Add capability to redo CCBs that fail [#1398] Review request
> for Ticket(s): 1398 Peer Reviewer(s): *** LIST THE TECH REVIEWER(S) /
> MAINTAINER(S) HERE *** Pull request to: *** LIST THE PERSON WITH PUSH
> ACCESS HERE *** Affected branch(es): develop Development branch: ticket-
> 1398 Base revision: d082d2fb2b26437bbe6860e8efff8479748378c2
> Personal repository: git://git.code.sf.net/u/elunlen/review
> 
> 
> 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
> 
> NOTE: Patch(es) contain lines longer than 80 characers
> 
> Comments (indicate scope for each "y" above):
> -
> Note:
> Logging and tracing is still tagged LLDTEST. This will be removed For testing
> use the demo programs as before
> 
> revision 011fc764956ff8892f577b44a7174384c85cc878
> Author:   Lennart Lund 
> Date: Tue, 16 Jan 2018 15:28:10 +0100
> 
> smf: Add capability to redo CCBs that fail [#1398]
> 
> Object modify is added
> ccbdemo_modify updated with demo code for object modify
> 
> Also some cleanup and bug fixes
> 
> 
> 
> revision 5c2931adb8b8f5d030b869a48e0fcb91ceeda3ac
> Author:   Lennart Lund 
> Date: Tue, 16 Jan 2018 15:14:42 +0100
> 
> smf: Add capability to redo CCBs that fail [#1398]
> 
> Object delete is added
> The ccbdemo1 command has changed name to ccbdemo_create A new
> command ccbdemo_delete is added A new command ccbdemo_modify is
> also added but is still dummy. This is in preparation for adding object modify
> 
> Also some cleanup and some bug fixes
> 
> 
> 
> revision c546f10cfe5344d071d4664099a62ab7f1f44209
> Author:   Lennart Lund 
> Date: Tue, 16 Jan 2018 15:14:42 +0100
> 
> smf: Add capability to

[devel] [PATCH 5/6] smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

Object modify is added
ccbdemo_modify updated with demo code for object modify

Also some cleanup and bug fixes
---
 .../smfd/imm_modify_config/add_operation_to_ccb.cc | 105 ++-
 .../smfd/imm_modify_config/add_operation_to_ccb.h  |  13 +-
 src/smf/smfd/imm_modify_config/attribute.cc| 320 ++---
 src/smf/smfd/imm_modify_config/attribute.h |  79 +++--
 src/smf/smfd/imm_modify_config/immccb.cc   | 265 -
 src/smf/smfd/imm_modify_config/immccb.h|  26 +-
 src/smf/smfd/imm_modify_demo/ccbdemo_create.cc |   4 +-
 src/smf/smfd/imm_modify_demo/ccbdemo_delete.cc |   4 +-
 src/smf/smfd/imm_modify_demo/ccbdemo_modify.cc | 119 ++--
 src/smf/smfd/imm_om_api/common/imm_attribute.h |   3 -
 src/smf/smfd/imm_om_api/om_ccb_object_create.h |   3 -
 11 files changed, 669 insertions(+), 272 deletions(-)

diff --git a/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc 
b/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc
index 9db1ab1c5..6948aa89c 100644
--- a/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc
+++ b/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc
@@ -49,7 +49,7 @@ bool IsResorceAbort(const SaImmCcbHandleT& ccbHandle) {
   const SaStringT *errString = nullptr;
   SaAisErrorT ais_rc = saImmOmCcbGetErrorStrings(ccbHandle, );
   if ((ais_rc == SA_AIS_OK) && (errString != nullptr)) {
-TRACE("%s: Error string: '%s'", __FUNCTION__, errString[0]);
+TRACE("LLDTEST %s: Error string: '%s'", __FUNCTION__, errString[0]);
 std::string err_str(errString[0]);
 if (err_str.find("IMM: Resource abort: ") != std::string::npos) {
   // Is Resource Abort
@@ -60,7 +60,7 @@ bool IsResorceAbort(const SaImmCcbHandleT& ccbHandle) {
   return rc;
 }
 
-int AddObjectCreateToCcb(const SaImmCcbHandleT& ccb_handle,
+int AddCreateToCcb(const SaImmCcbHandleT& ccb_handle,
const modelmodify::CreateDescriptor& create_descriptor) 
{
   TRACE_ENTER2("LLDTEST1: Parent '%s', Class '%s'",
create_descriptor.parent_name.c_str(),
@@ -84,7 +84,7 @@ int AddObjectCreateToCcb(const SaImmCcbHandleT& ccb_handle,
   //attribute values. This vector  must have the same scope as the
   //creator.
   modelmodify::AttributeHandler attributes();
-  if(attributes.AddAttributes(create_descriptor) == false) {
+  if(attributes.AddAttributesForObjectCreate(create_descriptor) == false) {
 LOG_NO("LLDTEST1 %s: SetAttributeValues() Fail", __FUNCTION__);
 recovery_info = modelmodify::kFail;
   }
@@ -125,16 +125,10 @@ int AddObjectCreateToCcb(const SaImmCcbHandleT& 
ccb_handle,
   return recovery_info;
 }
 
-// Add one delete operation to a CCB
-// Recovery:  BAD HANDLE; kRestartOm
-//FAILED OPERATION; kRestartOm or kFail
-//BUSY; An admin operation is ongoing on an object to be deleted
-//  We can try again to add the create
-// return: Recovery information. See immccb.h
-int AddObjectDeleteToCcb(const SaImmCcbHandleT& ccb_handle,
+int AddDeleteToCcb(const SaImmCcbHandleT& ccb_handle,
  const modelmodify::DeleteDescriptor&
  delete_descriptor) {
-
+  TRACE_ENTER2("LLDTEST");
   int recovery_info = modelmodify::kNotSet;
 
   // Setup an object deleter
@@ -150,13 +144,13 @@ int AddObjectDeleteToCcb(const SaImmCcbHandleT& 
ccb_handle,
 base::Sleep(base::MillisToTimespec(modelmodify::kBusyWait));
 continue;
   } else if (ais_rc == SA_AIS_ERR_BAD_HANDLE) {
-TRACE("LLDTEST1 %s: AddObjectDeleteToCcb() Recover, %s", __FUNCTION__,
+TRACE("LLDTEST1 %s: AddObjectDeleteToCcb() RestartOm, %s", 
__FUNCTION__,
   saf_error(ais_rc));
 recovery_info = modelmodify::kRestartOm;
 break;
-  } else if (SA_AIS_ERR_FAILED_OPERATION) {
+  } else if (ais_rc == SA_AIS_ERR_FAILED_OPERATION) {
 if (IsResorceAbort(ccb_handle)) {
-  TRACE("LLDTEST1 %s: AddObjectDeleteToCcb(), %s, kRestartOm",
+  TRACE("LLDTEST1 %s: AddObjectDeleteToCcb(), %s, RestartOm",
 __FUNCTION__, saf_error(ais_rc));
   recovery_info = modelmodify::kRestartOm;
   break;
@@ -167,6 +161,10 @@ int AddObjectDeleteToCcb(const SaImmCcbHandleT& ccb_handle,
   recovery_info = modelmodify::kFail;
   break;
 }
+  } else {
+// Unrecoverable Fail
+recovery_info = modelmodify::kFail;
+break;
   }
 }
 
@@ -181,5 +179,84 @@ int AddObjectDeleteToCcb(const SaImmCcbHandleT& ccb_handle,
 recovery_info = modelmodify::kFail;
   }
 
+  TRACE_LEAVE2("LLDTEST");
+  return recovery_info;
+}
+
+// Add one modify operation to a CCB
+// Recovery:  BAD HANDLE; kRestartOm
+//FAILED OPERATION; kRestartOm or kFail
+//BUSY; An admin operation is ongoing on an object to be modified
+//  We can try again to add the modify
+// return: Recovery information. See

[devel] [PATCH 4/6] smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

Object delete is added
The ccbdemo1 command has changed name to ccbdemo_create
A new command ccbdemo_delete is added
A new command ccbdemo_modify is also added but is still dummy. This is in
preparation for adding object modify

Also some cleanup and some bug fixes
---
 src/smf/Makefile.am|  95 --
 src/smf/smfd/imm_modify_config/README  |  13 +-
 .../{creator.cc => add_operation_to_ccb.cc}| 102 +--
 .../{creator.h => add_operation_to_ccb.h}  |  26 ++-
 src/smf/smfd/imm_modify_config/immccb.cc   | 188 
 src/smf/smfd/imm_modify_config/immccb.h|  18 +-
 src/smf/smfd/imm_modify_demo/Makefile  |   2 +-
 .../{ccbdemo1.cc => ccbdemo_create.cc} |  25 +--
 src/smf/smfd/imm_modify_demo/ccbdemo_delete.cc | 173 +++
 src/smf/smfd/imm_modify_demo/ccbdemo_modify.cc | 191 +
 10 files changed, 748 insertions(+), 85 deletions(-)
 rename src/smf/smfd/imm_modify_config/{creator.cc => add_operation_to_ccb.cc} 
(50%)
 rename src/smf/smfd/imm_modify_config/{creator.h => add_operation_to_ccb.h} 
(53%)
 rename src/smf/smfd/imm_modify_demo/{ccbdemo1.cc => ccbdemo_create.cc} (96%)
 create mode 100644 src/smf/smfd/imm_modify_demo/ccbdemo_delete.cc
 create mode 100644 src/smf/smfd/imm_modify_demo/ccbdemo_modify.cc

diff --git a/src/smf/Makefile.am b/src/smf/Makefile.am
index 05b35c0aa..673387db6 100644
--- a/src/smf/Makefile.am
+++ b/src/smf/Makefile.am
@@ -115,7 +115,7 @@ noinst_HEADERS += \
 # IMM configuration modifier
 noinst_HEADERS += \
src/smf/smfd/imm_modify_config/attribute.h \
-   src/smf/smfd/imm_modify_config/creator.h \
+   src/smf/smfd/imm_modify_config/add_operation_to_ccb.h \
src/smf/smfd/imm_modify_config/immccb.h
 
 # IMM OM C++ APIs
@@ -205,7 +205,7 @@ bin_osafsmfd_SOURCES = \
 # IMM configuration modifier
 bin_osafsmfd_SOURCES += \
src/smf/smfd/imm_modify_config/attribute.cc \
-   src/smf/smfd/imm_modify_config/creator.cc \
+   src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc \
src/smf/smfd/imm_modify_config/immccb.cc
 
 # IMM OM C++ APIs
@@ -258,29 +258,100 @@ endif
 # Demos and test
 if ENABLE_TESTS
 
-bin_PROGRAMS += bin/ccbdemo1
+bin_PROGRAMS += bin/ccbdemo_create bin/ccbdemo_delete bin/ccbdemo_modify
+
 dist_pkgimmxml_svc_DATA += \
src/smf/smfd/imm_modify_demo/democlass.xml
 
-#noinst_HEADERS +=
+# DEMO Create
+# ---
+bin_ccbdemo_create_CFLAGS = $(AM_CFLAGS) -Wformat=1
+
+bin_ccbdemo_create_CPPFLAGS = \
+   -DSA_EXTENDED_NAME_SOURCE \
+   $(AM_CPPFLAGS)
+
+bin_ccbdemo_create_SOURCES = \
+   src/smf/smfd/imm_modify_demo/ccbdemo_create.cc
+
+# IMM configuration modifier
+bin_ccbdemo_create_SOURCES += \
+   src/smf/smfd/imm_modify_config/attribute.cc \
+   src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc \
+   src/smf/smfd/imm_modify_config/immccb.cc
+
+# IMM OM C++ APIs
+bin_ccbdemo_create_SOURCES += \
+   src/smf/smfd/imm_om_api/common/common.cc \
+   src/smf/smfd/imm_om_api/common/imm_attribute.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_clear.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_handle.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_set.cc \
+   src/smf/smfd/imm_om_api/om_ccb_handle.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_create.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_delete.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_modify.cc \
+   src/smf/smfd/imm_om_api/om_handle.cc
+
+bin_ccbdemo_create_LDADD = \
+   lib/libosaf_common.la \
+   lib/libSaImmOm.la \
+   lib/libopensaf_core.la
+
+# DEMO Delete
+# ---
+bin_ccbdemo_delete_CFLAGS = $(AM_CFLAGS) -Wformat=1
+
+bin_ccbdemo_delete_CPPFLAGS = \
+   -DSA_EXTENDED_NAME_SOURCE \
+   $(AM_CPPFLAGS)
+
+bin_ccbdemo_delete_SOURCES = \
+   src/smf/smfd/imm_modify_demo/ccbdemo_delete.cc
+
+# IMM configuration modifier
+bin_ccbdemo_delete_SOURCES += \
+   src/smf/smfd/imm_modify_config/attribute.cc \
+   src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc \
+   src/smf/smfd/imm_modify_config/immccb.cc
+
+# IMM OM C++ APIs
+bin_ccbdemo_delete_SOURCES += \
+   src/smf/smfd/imm_om_api/common/common.cc \
+   src/smf/smfd/imm_om_api/common/imm_attribute.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_clear.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_handle.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_set.cc \
+   src/smf/smfd/imm_om_api/om_ccb_handle.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_create.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_delete.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_modify.cc \
+   src/smf/smfd/imm_om_api/om_handle.cc
+
+bin_ccbdemo_delete_LDADD = \
+   lib/libosaf_common.la \
+   lib/libSaImmOm.la \
+   lib/libopensaf_core.la
 
-bin_ccbdemo1_CFLAGS = $(AM_CFLAGS) -Wformat=1
+# DEMO Modify

[devel] [PATCH 6/6] smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

A test program, ccbhdl_test, is added to the demo directory that tests
combined CCBs, data ownership and reusage of created objects escpecially
the CCB handling object

* Fixed bugs found during testing. Fixed some review comments.
* Still some TODOs. These are more like comments about possible changes
  in the future. Will not be removed for now
* LLDTEST Tags still remains. Will be removed before pushing
* All review commits will be concatenated into one commit before pushing
---
 src/smf/Makefile.am|  37 +-
 .../smfd/imm_modify_config/add_operation_to_ccb.cc |   5 -
 src/smf/smfd/imm_modify_config/attribute.cc|   1 -
 src/smf/smfd/imm_modify_config/attribute.h |   2 -
 src/smf/smfd/imm_modify_config/immccb.cc   |  48 +--
 src/smf/smfd/imm_modify_config/immccb.h|  51 +--
 src/smf/smfd/imm_modify_demo/Makefile  |   2 +-
 src/smf/smfd/imm_modify_demo/ccbhdl_test.cc| 377 +
 src/smf/smfd/imm_om_api/om_admin_owner_clear.h |  11 +
 src/smf/smfd/imm_om_api/om_admin_owner_handle.h|   8 +
 src/smf/smfd/imm_om_api/om_admin_owner_set.h   |   6 +
 src/smf/smfd/imm_om_api/om_ccb_handle.cc   |   3 +-
 src/smf/smfd/imm_om_api/om_ccb_handle.h|   7 +
 src/smf/smfd/imm_om_api/om_ccb_object_create.h |   5 +
 src/smf/smfd/imm_om_api/om_ccb_object_delete.h |   5 +
 src/smf/smfd/imm_om_api/om_ccb_object_modify.h |   5 +
 src/smf/smfd/imm_om_api/om_handle.cc   |   8 +-
 src/smf/smfd/imm_om_api/om_handle.h|  13 +-
 18 files changed, 518 insertions(+), 76 deletions(-)
 create mode 100644 src/smf/smfd/imm_modify_demo/ccbhdl_test.cc

diff --git a/src/smf/Makefile.am b/src/smf/Makefile.am
index 673387db6..016da30c8 100644
--- a/src/smf/Makefile.am
+++ b/src/smf/Makefile.am
@@ -258,7 +258,7 @@ endif
 # Demos and test
 if ENABLE_TESTS
 
-bin_PROGRAMS += bin/ccbdemo_create bin/ccbdemo_delete bin/ccbdemo_modify
+bin_PROGRAMS += bin/ccbdemo_create bin/ccbdemo_delete bin/ccbdemo_modify 
bin/ccbhdl_test
 
 dist_pkgimmxml_svc_DATA += \
src/smf/smfd/imm_modify_demo/democlass.xml
@@ -368,4 +368,39 @@ bin_ccbdemo_modify_LDADD = \
lib/libSaImmOm.la \
lib/libopensaf_core.la
 
+# CCBHDL Test
+# ---
+bin_ccbhdl_test_CFLAGS = $(AM_CFLAGS) -Wformat=1
+
+bin_ccbhdl_test_CPPFLAGS = \
+   -DSA_EXTENDED_NAME_SOURCE \
+   $(AM_CPPFLAGS)
+
+bin_ccbhdl_test_SOURCES = \
+   src/smf/smfd/imm_modify_demo/ccbhdl_test.cc
+
+# IMM configuration modifier
+bin_ccbhdl_test_SOURCES += \
+   src/smf/smfd/imm_modify_config/attribute.cc \
+   src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc \
+   src/smf/smfd/imm_modify_config/immccb.cc
+
+# IMM OM C++ APIs
+bin_ccbhdl_test_SOURCES += \
+   src/smf/smfd/imm_om_api/common/common.cc \
+   src/smf/smfd/imm_om_api/common/imm_attribute.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_clear.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_handle.cc \
+   src/smf/smfd/imm_om_api/om_admin_owner_set.cc \
+   src/smf/smfd/imm_om_api/om_ccb_handle.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_create.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_delete.cc \
+   src/smf/smfd/imm_om_api/om_ccb_object_modify.cc \
+   src/smf/smfd/imm_om_api/om_handle.cc
+
+bin_ccbhdl_test_LDADD = \
+   lib/libosaf_common.la \
+   lib/libSaImmOm.la \
+   lib/libopensaf_core.la
+
 endif
diff --git a/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc 
b/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc
index 6948aa89c..6658182ff 100644
--- a/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc
+++ b/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc
@@ -18,11 +18,6 @@
 
 #include "add_operation_to_ccb.h"
 
-#if 1 // TODO(Lennart) Remove
-#include 
-#include 
-#endif
-
 #include 
 
 #include 
diff --git a/src/smf/smfd/imm_modify_config/attribute.cc 
b/src/smf/smfd/imm_modify_config/attribute.cc
index 72d5e8828..4b9f63817 100644
--- a/src/smf/smfd/imm_modify_config/attribute.cc
+++ b/src/smf/smfd/imm_modify_config/attribute.cc
@@ -44,7 +44,6 @@ using namespace modelmodify;
 //  - A numeric IMM type, SaImmValueTypeT
 // out: - A filled in numeric vector
 // return false on error. Type of error is logged in syslog
-// TODO(Lennart) Make static
 template
 static bool StringToNumericValue(const std::string& str_value,
   T& num_value, const SaImmValueTypeT imm_type) {
diff --git a/src/smf/smfd/imm_modify_config/attribute.h 
b/src/smf/smfd/imm_modify_config/attribute.h
index 7cb0e316c..61db1d8c8 100644
--- a/src/smf/smfd/imm_modify_config/attribute.h
+++ b/src/smf/smfd/imm_modify_config/attribute.h
@@ -32,8 +32,6 @@
 #ifndef ATTRVAL_HDL_H
 #define ATTRVAL_HDL_H
 
-// TODO(Lennart) Place within namespace modelmodify?
-
 // Convert std::string to SaNameT or SaAnyT.
 // Shall be used with AttributeDescriptor object where

[devel] [PATCH 0/6] Review Request for smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

Summary: smf: Add capability to redo CCBs that fail [#1398]
Review request for Ticket(s): 1398
Peer Reviewer(s): vijay@oracle.com
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-1398
Base revision: c5db5d0352af060cb94028b3b9b95e54d87cffbd
Personal repository: git://git.code.sf.net/u/elunlen/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

NOTE: Patch(es) contain lines longer than 80 characers

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

revision c2bf62c1257038ac31d62f17062e33bf39716d55
Author: Lennart Lund 
Date:   Mon, 22 Jan 2018 15:36:31 +0100

smf: Add capability to redo CCBs that fail [#1398]
vijay@oracle.com
A test program, ccbhdl_test, is added to the demo directory that tests
combined CCBs, data ownership and reusage of created objects escpecially
the CCB handling object

* Fixed bugs found during testing. Fixed some review comments.
* Still some TODOs. These are more like comments about possible changes
  in the future. Will not be removed for now
* LLDTEST Tags still remains. Will be removed before pushing
* All review commits will be concatenated into one commit before pushing



revision 8024683b6221ef612c99c683c95180469a90cab6
Author: Lennart Lund 
Date:   Mon, 22 Jan 2018 15:36:31 +0100

smf: Add capability to redo CCBs that fail [#1398]

Object modify is added
ccbdemo_modify updated with demo code for object modify

Also some cleanup and bug fixes



revision 146be8ba2412dd0962289fe28c64cfeb6af30c47
Author: Lennart Lund 
Date:   Mon, 22 Jan 2018 15:36:31 +0100

smf: Add capability to redo CCBs that fail [#1398]

Object delete is added
The ccbdemo1 command has changed name to ccbdemo_create
A new command ccbdemo_delete is added
A new command ccbdemo_modify is also added but is still dummy. This is in
preparation for adding object modify

Also some cleanup and some bug fixes



revision 34e85fe5972f653310d8059fc30c14152aca1340
Author: Lennart Lund 
Date:   Mon, 22 Jan 2018 15:36:31 +0100

smf: Add capability to redo CCBs that fail [#1398]

All types can be used to set attributes at create time
(SaAnyT and SaNameT added)
Help functions for converting SaAnyT and SaNameT to string is added
See SaAnytToString() and SaNametToString() in immccb.h



revision d50360224167780fda7fc3c1e60e0d5ea9e531d5
Author: Lennart Lund 
Date:   Mon, 22 Jan 2018 15:36:31 +0100

smf: Add capability to redo CCBs that fail [#1398]

Added all numeric types including SaTimeT
Still not implemented are SaNameT and SaAnyT



revision 409050761316b3d86d60ce88f7685c6420aec9c4
Author: Lennart Lund 
Date:   Mon, 22 Jan 2018 15:36:31 +0100

smf: Add capability to redo CCBs that fail [#1398]

Create a module for handling IMM CCB that implements all steps involved and
all rules regaring possible recovery and failing
Replace current CCB handling in SMF with useage of this module

NOTE: This is an early version that is not well tested and everything is not
  yet implemented. Also some refactoring can be done (redundency). In some
  places LLDTEST tagged traces, logs and printouts exists that will be 
removed
  or modified in a final version
- Only SaUint32T, SaInt32T and SaStringT is implemented.
  Means that IMM type SA_IMM_ATTR_SAUINT32T, SA_IMM_ATTR_SAINT32T and
  SA_IMM_ATTR_SASTRINGT can be set as value type
- A program called ccbdemo1 is included that can be used as an example and
  for some testing. This program is built and will be installed in an OpenSAF
  uml cluster if --enable-tests is set in configure
- An IMM class definition, democlass.xml, for ccbdemo1 is provided. This class
  must be installed before ccbdemo1 can be executed
  Example: # immcfg -f /hostfs/democlass.xml



Added Files:

 src/smf/config/democlass.xml
 src/smf/smfd/imm_modify_config/attribute.cc
 src/smf/smfd/imm_modify_config/attribute.h
 src/smf/smfd/imm_modify_config/creator.cc
 src/smf/smfd/imm_modify_config/creator.h
 src/smf/smfd/imm_modify_config/immccb.cc
 src/smf/smfd/imm_modify_config/immccb.h
 src/smf/smfd/imm_modify_config/README
 src/smf/smfd/imm_modify_demo/ccbdemo1.cc
 src/smf/smfd/imm_modify_demo/ccbdemo_delete.cc
 src/smf/smfd/imm_modify_demo/ccbdemo_modify.cc
 src/smf/smfd/imm_modify_demo/ccbhdl_test.cc
 src/smf/smfd/imm_modify_demo/democlass.xml
 src/smf/smfd/imm_modify_demo/Makefile
 src/smf/smfd/imm_om_api/common/common.cc

[devel] [PATCH 2/6] smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

Added all numeric types including SaTimeT
Still not implemented are SaNameT and SaAnyT
---
 src/smf/smfd/imm_modify_config/attribute.cc| 183 +++--
 src/smf/smfd/imm_modify_config/attribute.h | 118 +++-
 src/smf/smfd/imm_modify_demo/ccbdemo1.cc   |  71 --
 src/smf/smfd/imm_modify_demo/democlass.xml |   8 ++
 src/smf/smfd/imm_om_api/common/common.h|  41 ++
 src/smf/smfd/imm_om_api/common/imm_attribute.h |  29 
 6 files changed, 361 insertions(+), 89 deletions(-)

diff --git a/src/smf/smfd/imm_modify_config/attribute.cc 
b/src/smf/smfd/imm_modify_config/attribute.cc
index 8485cf2dc..93bd6f520 100644
--- a/src/smf/smfd/imm_modify_config/attribute.cc
+++ b/src/smf/smfd/imm_modify_config/attribute.cc
@@ -112,50 +112,43 @@ bool AttributeHandler::AddAttributes(const 
CreateDescriptor& create_descriptor)
   return rc;
 }
 
+// Add one attribute to a creator. Store the attribute and
+// its values until this object goes out of scope
+// Return false if fail
 bool AttributeHandler::AddAttribute(const AttributeDescriptor& attribute) {
   bool rc = true;
   SaImmValueTypeT value_type = attribute.value_type;
   switch (value_type) {
-case SA_IMM_ATTR_SAINT32T: {
-  SaInt32T numeric_value = 0;
-  std::vector num_values;
-  for (auto& value_str : attribute.values_as_strings) {
-if (StringToNumericValue
-(value_str, numeric_value, value_type) == false) {
-  LOG_NO("LLDTEST %s: '%s' could not be converted to a SaInt32T",
- __FUNCTION__, value_str.c_str());
-  rc = false;
-  break;
-}
-num_values.push_back(numeric_value);
-  }
-  std::unique_ptr CreatorAttribute =
-  std::unique_ptr(new SetAttribute(creator_));
-  CreatorAttribute->SetAttributeValues(attribute.attribute_name,
-  num_values);
-  set_attributesp_.push_back(std::move(CreatorAttribute));
+case SA_IMM_ATTR_SAINT32T:
+  rc = StoreNumericAttribute(attribute);
   break;
-}
-case SA_IMM_ATTR_SAUINT32T: {
-  SaUint32T num_value = 0;
-  std::vector numeric_values;
-  for (auto& value_str : attribute.values_as_strings) {
-if (StringToNumericValue
-(value_str, num_value, value_type) == false) {
-  LOG_NO("LLDTEST %s: '%s' could not be converted to a SaInt32T",
- __FUNCTION__, value_str.c_str());
-  rc = false;
-  break;
-}
-numeric_values.push_back(num_value);
-  }
-  std::unique_ptr CreatorAttribute =
-  std::unique_ptr(new SetAttribute(creator_));
-  CreatorAttribute->SetAttributeValues(attribute.attribute_name,
-  numeric_values);
-  set_attributesp_.push_back(std::move(CreatorAttribute));
+
+case SA_IMM_ATTR_SAUINT32T:
+  rc = StoreNumericAttribute(attribute);
   break;
-}
+
+case SA_IMM_ATTR_SAINT64T:
+  rc = StoreNumericAttribute(attribute);
+  break;
+
+case SA_IMM_ATTR_SAUINT64T:
+  rc = StoreNumericAttribute(attribute);
+  break;
+  
+case SA_IMM_ATTR_SATIMET:
+  rc = StoreNumericAttribute(attribute);
+  // rc = StoreTimeTAttribute(attribute);
+  break;
+
+case SA_IMM_ATTR_SAFLOATT:
+  rc = StoreNumericAttribute(attribute);
+  break;
+
+case SA_IMM_ATTR_SADOUBLET:
+  rc = StoreNumericAttribute(attribute);
+  break;
+
+
 case SA_IMM_ATTR_SASTRINGT: {
   std::unique_ptr CreatorAttribute =
   std::unique_ptr(new SetAttribute(creator_));
@@ -171,8 +164,40 @@ bool AttributeHandler::AddAttribute(const 
AttributeDescriptor& attribute) {
   return rc;
 }
 
+// Convert all values as a string for an attribute to numeric values.
+// Create a SetAttribute object and give that object the attribute name and its
+// numeric values. The SetAttribute object will store the attribute and give
+// the attribute name and values to the creator
+// Save the SetAttribute object in a vector so that the object will be in scope
+// until "this" object goes out of scope which shall happen when the creator
+// has given the create operation to IMM
+template
+bool AttributeHandler::
+StoreNumericAttribute(const AttributeDescriptor& attribute) {
+  bool rc = true;
+  SaImmValueTypeT value_type = attribute.value_type;
+  T numeric_value{0};
+  std::vector num_values;
+  for (auto& value_str : attribute.values_as_strings) {
+if (StringToNumericValue
+(value_str, numeric_value, value_type) == false) {
+  LOG_NO("LLDTEST %s: StringToNumericValue() Fail", __FUNCTION__);
+  rc = false;
+  break;
+}
+num_values.push_back(numeric_value);
+  }
+  if (rc == true) {
+std::unique_ptr CreatorAttribute =
+std::unique_ptr(new SetAttribute(creator_));
+  CreatorAttribute->SetAttributeValues(attribute.attribute_name,
+

[devel] [PATCH 3/6] smf: Add capability to redo CCBs that fail [#1398]

2018-01-22 Thread Lennart Lund

All types can be used to set attributes at create time
(SaAnyT and SaNameT added)
Help functions for converting SaAnyT and SaNameT to string is added
See SaAnytToString() and SaNametToString() in immccb.h
---
 src/smf/smfd/imm_modify_config/attribute.cc | 107 +++--
 src/smf/smfd/imm_modify_config/attribute.h  |  72 +++--
 src/smf/smfd/imm_modify_config/creator.cc   |   2 +-
 src/smf/smfd/imm_modify_config/immccb.cc|  24 +--
 src/smf/smfd/imm_modify_config/immccb.h |  59 ++-
 src/smf/smfd/imm_modify_demo/ccbdemo1.cc| 195 ++--
 src/smf/smfd/imm_modify_demo/democlass.xml  |  16 ++
 src/smf/smfd/imm_om_api/common/imm_attribute.cc |   9 +-
 8 files changed, 427 insertions(+), 57 deletions(-)

diff --git a/src/smf/smfd/imm_modify_config/attribute.cc 
b/src/smf/smfd/imm_modify_config/attribute.cc
index 93bd6f520..28dc6b76f 100644
--- a/src/smf/smfd/imm_modify_config/attribute.cc
+++ b/src/smf/smfd/imm_modify_config/attribute.cc
@@ -17,11 +17,6 @@
  */
 #include "attribute.h"
 
-#if 1 // TODO(Lennart) Remove
-#include 
-#include 
-#endif
-
 #include 
 
 #include 
@@ -104,7 +99,8 @@ static bool StringToNumericValue(const std::string& 
str_value,
   return rc;
 }
 
-bool AttributeHandler::AddAttributes(const CreateDescriptor& 
create_descriptor) {
+bool AttributeHandler::AddAttributes(const CreateDescriptor&
+ create_descriptor) {
   bool rc = true;
   for (auto& attribute_descriptor : create_descriptor.attributes) {
 rc = AddAttribute(attribute_descriptor);
@@ -134,7 +130,7 @@ bool AttributeHandler::AddAttribute(const 
AttributeDescriptor& attribute) {
 case SA_IMM_ATTR_SAUINT64T:
   rc = StoreNumericAttribute(attribute);
   break;
-  
+
 case SA_IMM_ATTR_SATIMET:
   rc = StoreNumericAttribute(attribute);
   // rc = StoreTimeTAttribute(attribute);
@@ -157,6 +153,15 @@ bool AttributeHandler::AddAttribute(const 
AttributeDescriptor& attribute) {
   set_attributesp_.push_back(std::move(CreatorAttribute));
   break;
 }
+
+case SA_IMM_ATTR_SANAMET:
+  StoreSaNametAttribute(attribute);
+  break;
+
+case SA_IMM_ATTR_SAANYT:
+  StoreSaAnytAttribute(attribute);
+  break;
+
 default:
   break;
   }
@@ -171,6 +176,13 @@ bool AttributeHandler::AddAttribute(const 
AttributeDescriptor& attribute) {
 // Save the SetAttribute object in a vector so that the object will be in scope
 // until "this" object goes out of scope which shall happen when the creator
 // has given the create operation to IMM
+
+// All Store methods:
+// For an attribute;
+//  - Convert all Values As a String to its real type and save
+//those values in a temporary vector.
+//  - Create a new SetAttribute object and give it the converted values
+//  - Store the SetAttribute object
 template
 bool AttributeHandler::
 StoreNumericAttribute(const AttributeDescriptor& attribute) {
@@ -179,6 +191,7 @@ StoreNumericAttribute(const AttributeDescriptor& attribute) 
{
   T numeric_value{0};
   std::vector num_values;
   for (auto& value_str : attribute.values_as_strings) {
+// Create a vector containing all values for this attribute
 if (StringToNumericValue
 (value_str, numeric_value, value_type) == false) {
   LOG_NO("LLDTEST %s: StringToNumericValue() Fail", __FUNCTION__);
@@ -188,15 +201,88 @@ StoreNumericAttribute(const AttributeDescriptor& 
attribute) {
 num_values.push_back(numeric_value);
   }
   if (rc == true) {
+// Store the attribute for the creator and give it to the creator
 std::unique_ptr CreatorAttribute =
 std::unique_ptr(new SetAttribute(creator_));
-  CreatorAttribute->SetAttributeValues(attribute.attribute_name,
-   num_values);
+CreatorAttribute->SetAttributeValues(attribute.attribute_name, num_values);
 set_attributesp_.push_back(std::move(CreatorAttribute));
   }
   return rc;
 }
 
+void AttributeHandler::
+StoreSaNametAttribute(const AttributeDescriptor& attribute) {
+  SaNameT name_value;
+  std::vector name_values;
+  for (auto& value_str : attribute.values_as_strings) {
+StringToSaNameT(value_str, _value);
+name_values.push_back(name_value);
+  }
+
+  std::unique_ptr CreatorAttribute =
+  std::unique_ptr(new SetAttribute(creator_));
+  CreatorAttribute->SetAttributeValues(attribute.attribute_name, name_values);
+  set_attributesp_.push_back(std::move(CreatorAttribute));
+}
+
+void AttributeHandler::
+StoreSaAnytAttribute(const AttributeDescriptor& attribute) {
+  // Note: For this type it is also needed to store the buffer pointed to
+  //   from the SaAnyT structure
+  SaAnyT any_value;
+  std::vector any_values;
+  //static inline void StringToSaAnyT(std::string str_anyt, SaAnyT* anyt_value)
+  for (auto& value_str : attribute.values_as_strings) {
+StringToSaAnyT(value_str, _value);
+any_values.push_back(any_value);
+  }
+
+

[devel] [PATCH 0/1] Review Request for nid: Use node address as node ID when slot_id is configured to zero [#2759]

2018-01-22 Thread Anders Widell

Summary: nid: Use node address as node ID when slot_id is configured to zero 
[#2759]
Review request for Ticket(s): 2759
Peer Reviewer(s): Ravi
Pull request to: 
Affected branch(es): develop
Development branch: ticket-2759
Base revision: c5db5d0352af060cb94028b3b9b95e54d87cffbd
Personal repository: git://git.code.sf.net/u/anders-w/review


Impacted area   Impact y/n

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


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

revision 0131fd2425dfcac9a9428f453d6d3d76cea65655
Author: Anders Widell 
Date:   Mon, 22 Jan 2018 13:20:04 +0100

nid: Use node address as node ID when slot_id is configured to zero [#2759]

Treat the value zero in /etc/opensaf/slot_id in the same way as if the file is
missing, i.e. use the TIPC address or IPv4 address as node ID.



Complete diffstat:
--
 00-README.conf  | 15 ---
 Makefile.am |  2 +-
 src/nid/opensafd.in |  3 +++
 3 files changed, 12 insertions(+), 8 deletions(-)


Testing Commands:
-
Configure the value zero in /etc/opensaf/slot_id


Testing, Expected Results:
--
The TIPC address or IPv4 address shall be used as Node ID.


Conditions of Submission:
-
Ack from reviewer(s), or on 2018-01-26 if no comments have been received.


Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.gitconfig file (i.e. user.name, user.email etc)

___ Your computer have a badly configured date and time; confusing the
the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results
for in-service upgradability test.

___ Your changes affect user manual and documentation, your patch series
do not contain the patch that updates the Doxygen manual.


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

[devel] [PATCH 1/1] nid: Use node address as node ID when slot_id is configured to zero [#2759]

2018-01-22 Thread Anders Widell

Treat the value zero in /etc/opensaf/slot_id in the same way as if the file is
missing, i.e. use the TIPC address or IPv4 address as node ID.
---
 00-README.conf  | 15 ---
 Makefile.am |  2 +-
 src/nid/opensafd.in |  3 +++
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/00-README.conf b/00-README.conf
index a8848e632..0fb4e9a81 100644
--- a/00-README.conf
+++ b/00-README.conf
@@ -230,9 +230,14 @@ The slot_id shall specify a unique value that represents a 
physical slot
 identifier for the node in a chassis environment. It is typically set to the
 same as the TIPC node ID.
 
-By default, the slot_id contains a value of 1. While configuring
-a new/second node in the cluster, this must be set to a unique/different 
-value - for e.g. a value of 2 as below:
+When using TIPC transport, or TCP transport with IPv4 node addresses, the
+slot_id file is not mandatory. If slot_id is missing or containts the value
+zero, OpenSAF will use the node's TIPC address or IPv4 address as Node ID.
+
+By default, the slot_id contains a value of 0, which means that the node's TIPC
+or IPv4 address will be used as node_id. If you configure slot_id to some other
+value than zero, you must you must make sure to use a unique slot_id value on
+each node. You can configure e.g. a value of 2 as below:
 % echo 2 > $pkgsysconfdir/slot_id
 
 Starting with OpenSAF version 5.0, the maximum supported value for slot_id is
@@ -240,10 +245,6 @@ Starting with OpenSAF version 5.0, the maximum supported 
value for slot_id is
 disabled). Prior to OpenSAF 5.0, the maximum supported value for slot_id was
 255.
 
-When using TIPC transport, or TCP transport with IPv4 node addresses, the
-slot_id file is not mandatory. If slot_id is missing, OpenSAF will use the
-node's address as Node ID.
-
 ***
 subslot_id
 
diff --git a/Makefile.am b/Makefile.am
index bcfd844cd..8f2687314 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -323,7 +323,7 @@ endif
 set-default-node-config:
@echo "*** Setting default controller node configuration ***"
echo "2" > $(DESTDIR)$(pkgsysconfdir)/chassis_id
-   echo "1" > $(DESTDIR)$(pkgsysconfdir)/slot_id
+   echo "0" > $(DESTDIR)$(pkgsysconfdir)/slot_id
echo "15" > $(DESTDIR)$(pkgsysconfdir)/subslot_id
echo "controller" > $(DESTDIR)$(pkgsysconfdir)/node_type
 
diff --git a/src/nid/opensafd.in b/src/nid/opensafd.in
index da4530969..7fa536761 100644
--- a/src/nid/opensafd.in
+++ b/src/nid/opensafd.in
@@ -175,6 +175,9 @@ generate_nodeid() {
return 0
fi
SLOT_ID=$(cat "$SLOT_ID_FILE")
+   if [ "$SLOT_ID" -eq "0" ]; then
+   return 0
+   fi
if [ "$SLOT_ID" -gt "4095" ] || [ "$SLOT_ID" -lt "1" ]
then
echo "SLOT ID Should be in the range of 1 to 4095"
-- 
2.13.3


--
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/3] Review Request for ntf: Checkpoint and cold sync reader information [#2757]

2018-01-22 Thread Lennart Lund

Hi Minh

Ack. I have not tested much

Have you tested using the reader API while running old version on standby and 
new version on active and vice versa (upgrade case)? Limitations?
PR documentation update?

Thanks
Lennart

> -Original Message-
> From: Minh Hon Chau
> Sent: den 22 januari 2018 05:19
> To: Lennart Lund ;
> srinivas.mangip...@oracle.com; Canh Van Truong
> 
> Cc: opensaf-devel@lists.sourceforge.net; Minh Hon Chau
> 
> Subject: [PATCH 0/3] Review Request for ntf: Checkpoint and cold sync
> reader information [#2757]
> 
> Summary: ntfd: Checkpoint reader to the standby when processes reader
> API requests [#2757]
> Review request for Ticket(s): 2757
> Peer Reviewer(s): Lennart, Srinivas, Canh
> Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
> Affected branch(es): develop
> Development branch: ticket-2757
> Base revision: ee105cb3bf44eee4e8785e3de7d24f907641e2ab
> Personal repository: git://git.code.sf.net/u/minh-chau/review
> 
> 
> 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
> 
> NOTE: Patch(es) contain lines longer than 80 characers
> 
> Comments (indicate scope for each "y" above):
> -
> *** EXPLAIN/COMMENT THE PATCH SERIES HERE ***
> 
> revision 74da3370accfa44a34a7abf9830ceaeae3ab5d4f
> Author:   Minh Chau 
> Date: Mon, 22 Jan 2018 15:08:59 +1100
> 
> ntftest: Add new test cases of suite 41 for cold sync and checkpoint of reader
> APIs [#2757]
> 
> 
> 
> revision ad38745b1c411bc52905725281c84c69e4147fef
> Author:   Minh Chau 
> Date: Mon, 22 Jan 2018 15:03:42 +1100
> 
> ntfd: Cold sync reader to the standby ntfd after rebooting the standby
> controller [#2757]
> 
> Assumpt that the reader information is updated to the standby ntfd via
> checkpoint
> upon reception of reader APIs requests. However, if the standby controller
> reboots
> and comes up, the standby ntfd still has none of readers information which is
> available at the active ntfd. Now if a switchover happens, the new active will
> not
> be able to process the reader APIs requests with existing reader handles.
> 
> This patch adds reader information as part of cold sync
> 
> 
> 
> revision 47cf18850e6819c2db4642eb1e639aff5f0d8282
> Author:   Minh Chau 
> Date: Mon, 22 Jan 2018 14:12:00 +1100
> 
> ntfd: Checkpoint reader to the standby when processes reader API requests
> [#2757]
> 
> When active ntfd receives reader API requests: ReaderIntialize, ReadNext,
> ReadFinalize, active ntfd does not update the readers information to the
> standby. Thus, either switchover or failover happens, the client can not
> continue to use the reader APIs, because there is no such reader information
> still available in the new active after switchover.
> 
> The patch does checkpoint reader information to the standby when
> completes
> processing reader APIs request.
> 
> 
> 
> Complete diffstat:
> --
>  src/ntf/agent/ntfa_mds.c   |  51 +--
>  src/ntf/apitest/tet_coldsync.c | 690
> -
>  src/ntf/common/ntfsv_enc_dec.c |  88 +-
>  src/ntf/common/ntfsv_enc_dec.h |  12 +-
>  src/ntf/ntfd/NtfAdmin.cc   | 145 +++--
>  src/ntf/ntfd/NtfAdmin.h|  17 +-
>  src/ntf/ntfd/NtfClient.cc  |  68 +++-
>  src/ntf/ntfd/NtfClient.h   |  11 +-
>  src/ntf/ntfd/NtfLogger.cc  |   2 +-
>  src/ntf/ntfd/NtfReader.cc  |  84 +++--
>  src/ntf/ntfd/NtfReader.h   |  13 +-
>  src/ntf/ntfd/ntfs_com.c| 105 +++
>  src/ntf/ntfd/ntfs_com.h|  25 +-
>  src/ntf/ntfd/ntfs_evt.c|  14 +-
>  src/ntf/ntfd/ntfs_mbcsv.c  | 287 ++---
>  src/ntf/ntfd/ntfs_mbcsv.h  |  16 +
>  src/ntf/ntfd/ntfs_mds.c|  42 +--
>  17 files changed, 1430 insertions(+), 240 deletions(-)
> 
> 
> Testing Commands:
> -
> Run all test cases of suite 41, and legacy suites
> 
> 
> Testing, Expected Results:
> --
> All pass
> 
> 
> Conditions of Submission:
> -
> ack from reviewers
> 
> 
> Arch  Built StartedLinux distro
> ---
> mipsn  n
> mips64  n  n
> x86 n  n
> x86_64  y  y
> powerpc n  n
> powerpc64   n  n
> 
> 
> Reviewer Checklist:
> ---
> [Submitters: make sure that your review doesn't trigger any checkmarks!]
> 
> 
> Your checkin has not passed review because

Re: [devel] [PATCH 0/5] Review Request for Add support for split brain prevention V2 [#64]

2018-01-22 Thread Anders Widell


See one comment below, marked AndersW2>

regards,

Anders Widell


On 01/22/2018 07:56 AM, Gary Lee wrote:

HI Anders/Hans


On 20/01/18 00:56, Anders Widell wrote:

Ack from me also, with comments:

* I think my major comment is that I had originally envisioned that 
you would use the "etcdctl lock" command (in the V3 API) and that the 
active SC would hold the lock for as long as it is active. The lock 
would not be needed for reading. Your approach of only creating the 
lock when you wish to change active controller could be fine though. 
However, you shouldn't need the lock for reading - only when you wish 
to update the active controller. Regarding the Watch: I think you 
should have the watch on the lock instead of (or in addition to) the 
data you are protecting. At a fail-over, the old standby would 
acquire the lock and wait a while to give the old active enough time 
to detect that a fail-over is pending (it notices that the lock has 
been created). The old active would then be able to remove the lock 
and prevent the fail-over from happening. We can look into this in 
the next iteration (next release) and keep it as it is for now.




[Gary] I will remove the opensaf_active_controller key and just have a 
key for the lock. The node is stored in the corresponding value. It's 
a lot simpler that way so I will do it for this release. The lock will 
not have a TTL and be persistent (until removed by another controller).


* You ought to utilise the test-and-set functionality in the etcd v2 
protocol, in the cases where you are changing the value of a key and 
know (think you know) the previous value. unlock is an example of 
this, fail-over probably also. We could add this later but I think 
you should at least extend the plugin API already now, so that it 
takes a "previous value" parameter where applicable.


* You have a try-again loop when you acquire the lock, but if the 
maximum number of retries have been done then you continue as if the 
lock was acquired successfully. It doesn't seem to be correct?


[Gary] Yes, will fix.



* It is not obvious (to me) that no more Watch thread can be created 
simultaneously. Could you add a flag that keeps track of if there is 
an existing thread, and add assert statements checking that there is 
no existing thread when you call MonitorActive() to create a new one?




[Gary] OK, will add a conditional statement.

* As Hans points out below, it seems that it is also possible that 
the watch thread could disappear silently in some error case.


[Gary] Will make it assert in that case.



* As already pointed out by Hans, we should store our keys in some 
directory in the etcd database, so that the same database can be used 
for other purposes as well. I think the plugin (shell script) should 
add a directory prefix to the key.




[Gary] Yes, good idea. The directory prefix will be handled by the 
plugin, in case the underlying key-value store doesn't handle 
directories etc.


AndersW> Split-brain should not be possible, however the current 
algorithm will not guarantee that the active SC will be in the 
largest partition in case TIPC connectivity is broken (partitioned). 
So it could happen that a single isolated node (from TIPC point of 
view) is the active SC, even though a larger TIPC partition exists. I 
think this could be solved by writing the size of the cluster into 
the lock. An existing active SC shall reject a fail-over if it is 
being initiated from a node in a smaller partition.




[Gary] Can we postpone this for the next release?


AndersW2> Yes, we can look at this in the next release. There are two 
things to look at: first, to make sure that the active SC doesn't "ping 
pong" back and forth between the two TIPC partitions. And secondly, if 
possible, to prefer to have the active SC in the largest TIPC partition.


Thanks
Gary







--
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/5] Review Request for smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 5/6] smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 4/6] smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 6/6] smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 0/6] Review Request for smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 2/6] smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 3/6] smf: Add capability to redo CCBs that fail [#1398]

[devel] [PATCH 0/1] Review Request for nid: Use node address as node ID when slot_id is configured to zero [#2759]

[devel] [PATCH 1/1] nid: Use node address as node ID when slot_id is configured to zero [#2759]

Re: [devel] [PATCH 0/3] Review Request for ntf: Checkpoint and cold sync reader information [#2757]

Re: [devel] [PATCH 0/5] Review Request for Add support for split brain prevention V2 [#64]

11 matches

Site Navigation

Mail list logo

Footer information