Hi  Hoang ,

On 6/30/2016 9:22 AM, Vo Minh Hoang wrote:
> Dear Mahesh,
>
> Now I am updating documents related to CKPT long DN.
> It will be submitted soon.
Ok you can ignore comment one.

I will provide patch by patch clarification & review comments, please 
find them  for  [PATCH 1 of 8]


Comment 1 :

==========================================================
@@ -26,6 +27,12 @@ static int __init_cpd(void)
  {
      NCS_LIB_REQ_INFO lib_create;

+    /* Enable extended SaNameT */
+    if (setenv("SA_ENABLE_EXTENDED_NAMES", "1", 1) != 0) {
+        LOG_ER("Failed to set environment variable: 
SA_ENABLE_EXTENDED_NAMES");
+        return m_LEAP_DBG_SINK(NCSCC_RC_FAILURE);
+    }
+
==========================================================

Can you please  provide   README about  long DN  Support , which will 
consist information such as `how steps to enable long DN setting`

similar to   LOG service,  please find  reference LOG service README 
Support Long DN
`https://sourceforge.net/p/opensaf/tickets/_discuss/thread/6ba7a0c9/8103/attachment/README_LONGDN_R2`


Comment 2 :

==========================================================
@@ -334,10 +334,10 @@ uint32_t cpd_amf_register(CPD_CB *cpd_cb

      }
      if (saAmfComponentRegister(cpd_cb->amf_hdl, &cpd_cb->comp_name, 
(SaNameT *)NULL) == SA_AIS_OK) {
-        TRACE_LEAVE2("cpd amf register success for 
%s",cpd_cb->comp_name.value);
+        TRACE_LEAVE2("cpd amf register success for %s", 
osaf_extended_name_borrow(&cpd_cb->comp_name));

==========================================================

I think `cpd_cb->comp_name` `cluster_node.nodeName` kind of variables 
are fill once and thy are consistent ,
so is it possible to store such variable once by using 
`osaf_extended_name_borrow()` when the intial value getting assigned?
this comment applies to all other consistent variables .

Comment 3 :

==========================================================

@@ -166,6 +162,9 @@ uint32_t cpd_ckpt_node_delete(CPD_CB *cb
          rc = NCSCC_RC_FAILURE;
      }

+    if (ckpt_node->ckpt_name != NULL)
+        free((void *)ckpt_node->ckpt_name);
+
==========================================================

Ideally frees can be part of m_MMGR_FREE_CPD_CKPT_INFO_NODE() , so that 
we can avoid multiple free calls ( code readability )
this comment applies to all other free chances.

Comment 4 :

==========================================================

@@ -123,8 +123,7 @@ typedef struct cpd_cpnd_info_node {
      uint32_t timer_state;
      bool ckpt_cpnd_scxb_exist;
      /* for imm */
-    SaNameT node_name;
-    SaNameT ckpt_name;
+    SaConstStringT node_name;
===========================================================

why ckpt_name removed form struct cpd_cpnd_info_node ?
is it no more used in this struct ?


Comment 5 :

It will be more useful to provide or change the cpsv_demo application to 
use LONG DN

/staging/samples/cpsv

-AVM

>
> Thank you and best regards,
> Hoang
>
> -----Original Message-----
> From: A V Mahesh [mailto:mahesh.va...@oracle.com]
> Sent: Wednesday, June 29, 2016 10:48 AM
> To: Hoang Vo <hoang.m...@dektech.com.au>
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 0 of 8] Review Request for CKPT: Support DNs longer than
> 255 bytes [#1574]
>
> Hi Hoang,
>
> On 6/23/2016 4:23 PM, Hoang Vo wrote:
>> Testing Commands:
>> -----------------
>> start all SCs and PLs
>> log in to Sc-1
>> run ckpttest
>> enable long DN setting
>     Can you please how steps to enable long DN setting.  it will be more
> useful to have  README about  long DN  Support similar to
>     LOG service,  please find  reference LOG service README Support Long DN
> `https://sourceforge.net/p/opensaf/tickets/_discuss/thread/6ba7a0c9/8103/att
> achment/README_LONGDN_R2`
>
>
> -AVM
>
>
>
> On 6/23/2016 4:23 PM, Hoang Vo wrote:
>> Summary: CKPT: Support DNs longer than 255 bytes [#1574]
>> Review request for Trac Ticket(s): 1574
>> Peer Reviewer(s): mahesh.va...@oracle.com; anders.wid...@ericsson.com
>> Pull request to: mahesh.va...@oracle.com
>> Affected branch(es): default
>> Development branch: default
>>
>> --------------------------------
>> Impacted area       Impact y/n
>> --------------------------------
>>    Docs                    n
>>    Build system            n
>>    RPM/packaging           n
>>    Configuration files     n
>>    Startup scripts         n
>>    SAF services            y
>>    OpenSAF services        n
>>    Core libraries          n
>>    Samples                 n
>>    Tests                   y
>>    Other                   n
>>
>>
>> Comments (indicate scope for each "y" above):
>> ---------------------------------------------
>>
>> changeset 729f0b7318a9e628a00ed6ab0617c85d4527aa73
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      cpd: Add support for extended SaNameT [#1574]
>>
>> changeset c2fae0887466adc3414fa64506832122896e6070
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      cpnd: Add support for extended SaNameT v1 [#1574]
>>
>> changeset 98a1ed77d1de469b95022d5ea302b1e3d94b949b
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      cpa: Add support for extended SaNameT v1 [#1574]
>>
>> changeset adb30ae18b5972a4948403cabe379d5546ccdbea
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      cpsv: Add new message to support extended SaNameT v1 [#1574] New
> messages
>>      supporting extended SaNameT are introduce. Encoding and decoding
> funtions
>>      for them are also included.
>>
>> changeset dd4916df11a3e50a76f9002b00fefd63364f7599
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      cpd: Add new mbcsv messages supporting extended SaNameT v1 [#1574]
> New MBCSV
>>      messages supporting extended SaNameT and their encoding/decoding
> functions
>>      are included.
>>
>> changeset f091e95968329ed246c613537c636efe1508f3e8
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      cpsv: Apply new messages supporting extended SaNameT to CPD, CPND,
> and CPA
>>      v1 [#1574]
>>
>> changeset d1bc0b21ac74f3eb2a6fff7c3d46dd7ef478ec92
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      ckpt: Add new test cases to verify long DN feature on CPSV v1
> [#1574]
>> changeset 4d297fdba6e02c0501ad9872f98b85372bb6f6ce
>> Author:      Hoang Vo <hoang.m...@dektech.com.au>
>> Date:        Thu, 23 Jun 2016 17:42:12 +0700
>>
>>      imported patch 1574_cpnd_support_recover_shm_version_0_v2.patch
>>
>>
>> Complete diffstat:
>> ------------------
>>    osaf/libs/agents/saf/cpa/Makefile.am      |    1 +
>>    osaf/libs/agents/saf/cpa/cpa_api.c        |   50 ++++---
>>    osaf/libs/agents/saf/cpa/cpa_db.c         |    2 +
>>    osaf/libs/agents/saf/cpa/cpa_mds.c        |    4 +-
>>    osaf/libs/agents/saf/cpa/cpa_proc.c       |    2 +-
>>    osaf/libs/common/cpsv/cpsv_evt.c          |  505
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +--
>>    osaf/libs/common/cpsv/include/cpa.h       |    1 +
>>    osaf/libs/common/cpsv/include/cpa_cb.h    |    2 +-
>>    osaf/libs/common/cpsv/include/cpa_proc.h  |    2 +-
>>    osaf/libs/common/cpsv/include/cpd.h       |    1 +
>>    osaf/libs/common/cpsv/include/cpd_cb.h    |   17 +-
>>    osaf/libs/common/cpsv/include/cpd_imm.h   |    4 +-
>>    osaf/libs/common/cpsv/include/cpd_proc.h  |    2 +-
>>    osaf/libs/common/cpsv/include/cpnd.h      |    1 +
>>    osaf/libs/common/cpsv/include/cpnd_cb.h   |    5 +-
>>    osaf/libs/common/cpsv/include/cpnd_init.h |    3 +-
>>    osaf/libs/common/cpsv/include/cpsv_evt.h  |   24 +++
>>    osaf/libs/common/cpsv/include/cpsv_shm.h  |   24 +++-
>>    osaf/services/saf/cpsv/cpd/Makefile.am    |    1 +
>>    osaf/services/saf/cpsv/cpd/cpd_amf.c      |    7 +-
>>    osaf/services/saf/cpsv/cpd/cpd_db.c       |  110 +++++++++-------
>>    osaf/services/saf/cpsv/cpd/cpd_evt.c      |  107 +++++++++++-----
>>    osaf/services/saf/cpsv/cpd/cpd_imm.c      |  268
> ++++++++++++++++++++++++++++++------------
>>    osaf/services/saf/cpsv/cpd/cpd_main.c     |    7 +
>>    osaf/services/saf/cpsv/cpd/cpd_mbcsv.c    |   31 ++++-
>>    osaf/services/saf/cpsv/cpd/cpd_mds.c      |   85 ++++++++++++-
>>    osaf/services/saf/cpsv/cpd/cpd_proc.c     |  197
> +++++++++++++++++--------------
>>    osaf/services/saf/cpsv/cpd/cpd_red.c      |    6 +-
>>    osaf/services/saf/cpsv/cpd/cpd_sbevt.c    |   57 +++-----
>>    osaf/services/saf/cpsv/cpnd/Makefile.am   |    1 +
>>    osaf/services/saf/cpsv/cpnd/cpnd_db.c     |    6 +-
>>    osaf/services/saf/cpsv/cpnd/cpnd_evt.c    |   91 +++++++++----
>>    osaf/services/saf/cpsv/cpnd/cpnd_main.c   |    7 +
>>    osaf/services/saf/cpsv/cpnd/cpnd_mds.c    |   84 ++++++++++++-
>>    osaf/services/saf/cpsv/cpnd/cpnd_proc.c   |   88 ++++++++++---
>>    osaf/services/saf/cpsv/cpnd/cpnd_res.c    |  823
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++--------------------------------
>>    tests/cpsv/Makefile.am                    |    1 +
>>    tests/cpsv/test_cpa.c                     |  180
> ++++++++++++++++++++++++++++-
>>    tests/cpsv/test_cpa_util.c                |   15 ++
>>    tests/cpsv/test_cpsv.h                    |    6 +
>>    tests/cpsv/test_cpsv_conf.h               |    3 +
>>    41 files changed, 2205 insertions(+), 626 deletions(-)
>>
>>
>> Testing Commands:
>> -----------------
>> start all SCs and PLs
>> log in to Sc-1
>> run ckpttest
>> enable long DN setting
>> run ckpttest again
>> log in to PL-3
>> run ckpttest
>> enable long DN setting
>> run ckpttest again
>>
>> Testing, Expected Results:
>> --------------------------
>> all ckpttest test cases return OK
>>
>> Conditions of Submission:
>> -------------------------
>> ACK from maintainer
>>
>> Arch      Built     Started    Linux distro
>> -------------------------------------------
>> mips        n          n
>> mips64      n          n
>> x86         n          n
>> x86_64      y          y
>> powerpc     n          n
>> powerpc64   n          n
>>
>>
>> Reviewer Checklist:
>> -------------------
>> [Submitters: make sure that your review doesn't trigger any checkmarks!]
>>
>>
>> Your checkin has not passed review because (see checked entries):
>>
>> ___ Your RR template is generally incomplete; it has too many blank
> entries
>>       that need proper data filled in.
>>
>> ___ You have failed to nominate the proper persons for review and push.
>>
>> ___ Your patches do not have proper short+long header
>>
>> ___ You have grammar/spelling in your header that is unacceptable.
>>
>> ___ You have exceeded a sensible line length in your
> headers/comments/text.
>> ___ You have failed to put in a proper Trac Ticket # into your commits.
>>
>> ___ You have incorrectly put/left internal data in your comments/files
>>       (i.e. internal bug tracking tool IDs, product names etc)
>>
>> ___ You have not given any evidence of testing beyond basic build tests.
>>       Demonstrate some level of runtime or other sanity testing.
>>
>> ___ You have ^M present in some of your files. These have to be removed.
>>
>> ___ You have needlessly changed whitespace or added whitespace crimes
>>       like trailing spaces, or spaces before tabs.
>>
>> ___ You have mixed real technical changes with whitespace and other
>>       cosmetic code cleanup changes. These have to be separate commits.
>>
>> ___ You need to refactor your submission into logical chunks; there is
>>       too much content into a single commit.
>>
>> ___ You have extraneous garbage in your review (merge commits etc)
>>
>> ___ You have giant attachments which should never have been sent;
>>       Instead you should place your content in a public tree to be pulled.
>>
>> ___ You have too many commits attached to an e-mail; resend as threaded
>>       commits, or place in a public tree for a pull.
>>
>> ___ You have resent this content multiple times without a clear indication
>>       of what has changed between each re-send.
>>
>> ___ You have failed to adequately and individually address all of the
>>       comments and change requests that were proposed in the initial
> review.
>> ___ You have a misconfigured ~/.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.
>>
>


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to