Re: [devel] [PATCH 1/1] smfd: Fix incorrect handling of SMFND NCSMDS_UP/DOWN events [#2821]

[Syam Prasad Talluri]

Hi Nguyen,

Reviewed the Patch. Ack from my side.

Thanks,
Syam.

-Original Message-
From: Nguyen Luu [mailto:nguyen.tk@dektech.com.au] 
Sent: Monday, April 9, 2018 7:56 AM
To: Lennart Lund ; Vijay Roy 
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [devel] [PATCH 1/1] smfd: Fix incorrect handling of SMFND 
NCSMDS_UP/DOWN events [#2821]

Hi,

I intend to push this patch by the end of today if there are no more comments.

Thanks,
Nguyen

On 4/6/2018 10:54 AM, Nguyen Luu wrote:
> Hi Lennart,
>
> Thank you for your comment.
>
> You suggested that the changed files be reformatted following Google 
> Coding Style (i.e space-indented); but according to OpenSAF Coding 
> Rules 
> ,
>  C code shall follow Linux kernel Coding Style (i.e tab-indented).
> It seems that the old code line used mixed tabs and spaces, so that's 
> probably why you see my new code line as incorrectly indented. I used 
> 'tabs' actually.
>
> Can you check again and confirm the coding style?
>
> Thanks,
> Nguyen
>
> On 4/5/2018 8:38 PM, Lennart Lund wrote:
>> Hi Nguyen,
>>
>> Ack with comments. See below [Lennart]
>>
>> Thanks
>> Lennart
>>
>>> -Original Message-
>>> From: Nguyen Luu [mailto:nguyen.tk@dektech.com.au]
>>> Sent: den 28 mars 2018 10:08
>>> To: Lennart Lund ; vijay@oracle.com
>>> Cc: opensaf-devel@lists.sourceforge.net; Nguyen Tran Khoi Luu 
>>> 
>>> Subject: [PATCH 1/1] smfd: Fix incorrect handling of SMFND 
>>> NCSMDS_UP/DOWN events [#2821]
>>>
>>> Current handling of SMFND DOWN event does not take into account 
>>> failed SMFND UP event, which could eventually result in an inexact 
>>> view of the actual number of SMFND nodes in the cluster if, for 
>>> example, a node happened to be DOWN and UP twice, and the first UP 
>>> event somehow failed.
>>> ---
>>>   src/smf/smfd/smfd_evt.c   | 7 ---
>>>   src/smf/smfd/smfd_smfnd.c | 9 +
>>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/smf/smfd/smfd_evt.c b/src/smf/smfd/smfd_evt.c index 
>>> 32f83fd..6f60c13 100644
>>> --- a/src/smf/smfd/smfd_evt.c
>>> +++ b/src/smf/smfd/smfd_evt.c
>>> @@ -1,6 +1,7 @@
>>>   /*  -*- OpenSAF  -*-
>>>    *
>>>    * (C) Copyright 2008 The OpenSAF Foundation
>>> + * Copyright (C) 2018 Ericsson AB. 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 @@ -86,7 +87,7 @@ static void 
>>> proc_mds_info(smfd_cb_t *cb, SMFSV_EVT
>>> *evt)
>>>
>>>   if (mds_info->svc_id == NCSMDS_SVC_ID_SMFND) {
>>>   if (smfnd_up(mds_info->node_id, mds_info->dest,
>>> - mds_info->rem_svc_pvt_ver) ==
>>> SA_AIS_OK)
>> [Lennart] Format: The following line has incorrect indentation and is 
>> probably too long. It is Ok to reformat this file according to Google 
>> style guide
>>> +    mds_info-
 rem_svc_pvt_ver) == NCSCC_RC_SUCCESS)
>>>   cb->no_of_smfnd++;
>>>   else
>>>   LOG_WA("%s: SMFND UP failed", __FUNCTION__); @@ 
>>> -100,8 +101,8 @@ static void proc_mds_info(smfd_cb_t *cb, SMFSV_EVT 
>>> *evt)
>>>   }
>>>
>>>   if (mds_info->svc_id == NCSMDS_SVC_ID_SMFND) {
>>> -    smfnd_down(mds_info->node_id);
>>> -    cb->no_of_smfnd--;
>>> +    if (smfnd_down(mds_info->node_id) ==
>>> NCSCC_RC_SUCCESS)
>>> +    cb->no_of_smfnd--;
>>>   }
>>>   break;
>>>
>>> diff --git a/src/smf/smfd/smfd_smfnd.c b/src/smf/smfd/smfd_smfnd.c 
>>> index c48adb2..5a64507 100644
>>> --- a/src/smf/smfd/smfd_smfnd.c
>>> +++ b/src/smf/smfd/smfd_smfnd.c
>>> @@ -1,6 +1,7 @@
>>>   /*  OpenSAF
>>>    *
>>>    * (C) Copyright 2008 The OpenSAF Foundation
>>> + * Copyright (C) 2018 Ericsson AB. 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 @@ -212,6 +213,14 @@ uint32_t 
>>> smfnd_down(SaClmNodeIdT i_node_id)
>>>   /* Update the node info */
>>>   while (smfnd != NULL) {
>>>   if (smfnd->clmInfo.nodeId == i_node_id) {
>>> +    /* Check if the node state was already Down,
>>> + * probably due to previous failed SMFND UP event
>>> + */
>>> +    if (smfnd->nd_state == ndDown) {
>> [Lennart] Formatting: The following line is too long. It is Ok to 
>> reformat this file to follow Google style guide
>>> +    

Re: [devel] [PATCH 1/1] dtm: Document update for transportd.conf and osaflog options [#2820]

[Syam Prasad Talluri]

Anders,

Thanks for your comments.
Will commit the patch with code changes specified by you along with doc updates.

Thanks,
Syam.
-Original Message-
From: Anders Widell [mailto:anders.wid...@ericsson.com] 
Sent: Wednesday, March 28, 2018 7:23 PM
To: syam-talluri 
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1/1] dtm: Document update for transportd.conf and osaflog 
options [#2820]

Ack with comments, marked AndersW> below.

regards,

Anders Widell


On 03/27/2018 02:26 PM, syam-talluri wrote:
>   modified:   00-README.conf
>   modified:   src/dtm/README
> ---
>   00-README.conf | 15 +++
>   src/dtm/README | 23 +++
>   2 files changed, 38 insertions(+)
>
> diff --git a/00-README.conf b/00-README.conf index 94cb3d3..5f3927b 
> 100644
> --- a/00-README.conf
> +++ b/00-README.conf
> @@ -102,6 +102,21 @@ DTM_MCAST_ADDR=file:/var/lib/peer_ip_addresses.txt
>   DTM_MCAST_ADDR=dns:peers.opensaf.org
>   
>   
> **
> *
> +transportd.conf
> +
> +This file contains the configuration for transportd Service. This 
> +file has to be modified in the following cases:
> +
> +(a) To override the default max log file size (5242880 bytes) and
> +default number of log backups (9 log backups).

AndersW> Is case (a) really a separate case? I think you should remove
it and just keep case (b) and (c) below. You can mention the default values in 
case (b) and (c).

> +(b) To override the default max log file size, enable the option
> +TRANSPORT_MAX_LOG_FILESIZE by removing # in the starting of
> +the line and update its value to the required number of bytes.
> +(c) To override the default number of log backups, enable the option
> +TRANSPORT_NO_OF_BACKUP_LOG_FILES by removing # in the starting of
> +the line and update its value to the required number of backups.

AndersW> I realize now that the names of these two configuration options
are different from the command-line options of the osaflog tool. This can be 
confusing since they control the same settings. Rename 
TRANSPORT_MAX_LOG_FILESIZE to TRANSPORT_MAX_FILE_SIZE and 
TRANSPORT_NO_OF_BACKUP_LOG_FILES to TRANSPORT_MAX_BACKUPS. You will of course 
update the names in transportd.conf and log_server.cc too.

> +
> +***
>   nid.conf
>   
>   This file contains global configuration for OpenSAF. This file
> diff --git a/src/dtm/README b/src/dtm/README
> index bccd8fe..814e49a 100644
> --- a/src/dtm/README
> +++ b/src/dtm/README
> @@ -168,3 +168,26 @@ in dtmd.conf (see CONFIGURATION above) and restart the 
> cluster.
>
>   For fatal errors, syslog is used.
>   
> +Command Line Tool:
> +
> +Use the osaflog command line tool to change the log server configuration.
> +osaflog tool supports the below options.
> +
> +Usage: osaflog [OPTION] [LOGSTREAM]
> +
> +print the messages stored on disk for the specified
> +LOGSTREAM. When a LOGSTREAM argument is specified, the option
> +--flush is implied.
> +
> +Opions:
> +
> +--flush  Flush all buffered messages in the log server to
> + disk even when no LOGSTREAM is specified
> +--print  print the messages stored on disk for the
> + specified LOGSTREAM.This option is default
> + when no option is specified.
> +--max-file-size  Set the maximum size (in bytes) of the log file
> + before the log is rotated.
> +--max-backupsSet the maximum number of backup files to keep
> + when rotating the log.

AndersW> To make it more visible that the last two options require 
arguments,
change the description of them to:

--max-file-size=SIZE  Set the maximum size of the log file to SIZE bytes.
   The log file will be rotated when it exceeds this size.
--max-backups=NUM Set the maximum number of backup files to retain during
   log rotation to NUM.

Also make the same change in osaflog.cc

> +


--
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: do not dereference null pointer [#2791]

[Syam Prasad Talluri]

Hi Gary,

I tried to analyze the flows in MDS code to figure out the root cause, but it 
is of vain.  
Can you please give me the reproducible scenario, I will try out by adding 
debug logs

Thanks,
Syam.

-Original Message-
From: Ravi Sekhar Reddy Konda 
Sent: Monday, March 19, 2018 10:28 AM
To: Gary Lee 
Cc: minh.c...@dektech.com.au; opensaf-devel@lists.sourceforge.net
Subject: Re: [devel] [PATCH 1/1] amf: do not dereference null pointer [#2791]

Fine Gary, my only worry is this is a crucial MDS issue which might get un 
noticed.
Please raise a MDS ticket referring to this ticket along with MDS logs, we will 
try to look into it.

>From AMF side, I am fine to have sanity check so ACK for the patch 

Thanks,
Ravi

-Original Message-
From: Gary Lee [mailto:gary@dektech.com.au]
Sent: Friday, March 16, 2018 6:24 PM
To: Ravi Sekhar Reddy Konda 
Cc: hans.nordeb...@ericsson.com; minh.c...@dektech.com.au; 
opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1/1] amf: do not dereference null pointer [#2791]

Hi Ravi

Yes, I'm not very familiar with the mds code so I haven't fixed it there.
Should we have this sanity check in AMF anyway?

Thanks
Gary

> On 16 Mar 2018, at 11:43 pm, Ravi Sekhar Reddy Konda 
>  wrote:
> 
> Hi Gary,
> 
> The only case I see where MDS can return NCSCC_RC_SUCCESS and still 
> sndrsp.o_rsp is NULL is in the case of Timeouts.
> In this case the fix might avoid the core, but the core problem will 
> still be there and it might affect other flows or services also I 
> think the better solution is to return NCSCC_RC_REQ_TIMOUT from the 
> MDS and let the Application handle it
> 
> Thanks,
> Ravi
> 
> -Original Message-
> From: Gary Lee [mailto:gary@dektech.com.au]
> Sent: Thursday, March 01, 2018 11:02 AM
> To: hans.nordeb...@ericsson.com; ravisekhar.ko...@oracle.com; 
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net; Gary Lee 
> 
> Subject: [PATCH 1/1] amf: do not dereference null pointer [#2791]
> 
> Callers of ava_mds_send() assume *o_msg is not null, if the return code is 
> NCSCC_RC_SUCCESS.
> ---
> src/amf/agent/ava_mds.cc | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/src/amf/agent/ava_mds.cc b/src/amf/agent/ava_mds.cc index 
> 440885332..cd139365d 100644
> --- a/src/amf/agent/ava_mds.cc
> +++ b/src/amf/agent/ava_mds.cc
> @@ -378,6 +378,10 @@ uint32_t ava_mds_send(AVA_CB *cb, AVSV_NDA_AVA_MSG 
> *i_msg,
>   /* retrieve the response */
>   *o_msg = (AVSV_NDA_AVA_MSG *)mds_info.info.svc_send.info.sndrsp.o_rsp;
>   mds_info.info.svc_send.info.sndrsp.o_rsp = 0;
> +  if (*o_msg == nullptr) {
> +LOG_ER("No response received");
> +rc = NCSCC_RC_FAILURE;
> +  }
> }
>   } else
> /* just a 'normal' send */
> --
> 2.14.1


--
Check out the vibrant tech community on one of the world's most engaging tech 
sites, Slashdot.org! 
https://urldefense.proofpoint.com/v2/url?u=http-3A__sdm.link_slashdot=DwICAg=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=V-rHpog7bglMV_qtz_u-J_IeS6G1w2MSO-XRrmj38iQ=nCoVGgacsr0qVPRrRS9qhe9XYM3zYGiz2WKBV6DklJI=IVTqrKUno3ASikPGTEb7t-EnsbcgW8ooZuTn3z8ucKw=
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.sourceforge.net_lists_listinfo_opensaf-2Ddevel=DwICAg=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=V-rHpog7bglMV_qtz_u-J_IeS6G1w2MSO-XRrmj38iQ=nCoVGgacsr0qVPRrRS9qhe9XYM3zYGiz2WKBV6DklJI=BRZ9PUG7Pa1_cQ3W1vGW23wpGubj6q7mCG88l8O8Qco=

--
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 2/2] dtm: Added following options --max-backups and --max-file-size to osaflog tool and in transportd [#2731]

[Syam Prasad Talluri]

Thanks Anders. Please see my responses inline below marked <> 

Will fix cpplint warnings while incorporating review comments.

regards,
Syam.
-Original Message-
From: Anders Widell [mailto:anders.wid...@ericsson.com] 
Sent: Tuesday, February 13, 2018 9:38 PM
To: syam-talluri 
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 2/2] dtm: Added following options --max-backups and 
--max-file-size to osaflog tool and in transportd [#2731]

Hi!

General comment: I notice that you have changed the interface between the 
osaflog command and the osaftransportd server from a simple text-based format 
into a binary format defined by several structures and enums. This makes the 
code more complicated and error-prone, so please change it back to the simple 
text-based interface. Requests are text strings starting with a question mark, 
and replies are text strings starting with an exclamation mark. E.g. when 
running the command "osaflog --max-file-size 1000" you would send the message 
"?max-file-size 1000" to the server, and the server could reply back with 
"!max-file-size 1000" when the operation has completed (1000 here would mean 
that the new file size limit is now 1000 - i.e. the operation was successful).
<>
In general it is better to have structure format for any event communication as 
it will be useful in extending the event with more options in future.
If you don’t see any new options getting added for osaflog in future I will 
change it to text format.

See more comments inline below, marked AndersW>

regards,
Anders Widell

On 02/12/2018 01:16 PM, syam-talluri wrote:
> ---
>   src/dtm/common/osaflog_protocol.h |  15 +++
>   src/dtm/tools/osaflog.cc  | 262 
> ++
>   src/dtm/transport/log_server.cc   |  60 -
>   src/dtm/transport/log_server.h|  10 +-
>   src/dtm/transport/main.cc |   5 +-
>   src/dtm/transport/transportd.conf |   8 +-
>   6 files changed, 293 insertions(+), 67 deletions(-)
>
> diff --git a/src/dtm/common/osaflog_protocol.h 
> b/src/dtm/common/osaflog_protocol.h
> index 61e9f6f..9723fd5 100644
> --- a/src/dtm/common/osaflog_protocol.h
> +++ b/src/dtm/common/osaflog_protocol.h
> @@ -24,6 +24,21 @@
>   
>   namespace Osaflog {
>   
> +enum cmd { FLUSH, MAXBACKUPS, MAXFILESIZE}; enum cmdreply { RFLUSH = 
> +101, RMAXBACKUPS, RMAXFILESIZE, FAILURE}; struct cmd_osaflog {
> +char marker[4];
> +enum cmd  m_cmd;// Command Enum
> +size_tm_value; // Value based on the command
> +};
> +
> +
> +struct cmd_osaflog_resp
> +{
> +enum cmdreply  m_cmdreply;// Command Enum
> +};
> +
>   static constexpr const char* kServerSocketPath =
>   PKGLOCALSTATEDIR "/osaf_log.sock";
>   
> diff --git a/src/dtm/tools/osaflog.cc b/src/dtm/tools/osaflog.cc index 
> 3ce66f4..aefff81 100644
> --- a/src/dtm/tools/osaflog.cc
> +++ b/src/dtm/tools/osaflog.cc
> @@ -20,6 +20,7 @@
>   #include 
>   #include 
>   #include 
> +#include 

AndersW> Includes should be in alphabetical order, so move the getopt.h
include up to its correct position.
<> Ok, I will change it 
>   #include 
>   #include 
>   #include 
> @@ -38,7 +39,9 @@
>   namespace {
>   
>   void PrintUsage(const char* program_name);
> -bool Flush();
> +bool Flush(bool flush_done);

AndersW> It is not clean code to add a boolean parameter to a function, 
which indicates that the function shall not be executed. Remove the 
flush_done parameter, and simply don't call the function if it shouldn't 
be executed.
<> Ok, will change it.

> +bool MaxTraceFileSize(size_t maxfilesize);
> +bool NoOfBackupFiles(size_t nooffiles);
>   base::UnixServerSocket* CreateSocket();
>   uint64_t Random64Bits(uint64_t seed);
>   bool PrettyPrint(const std::string& log_stream);
> @@ -53,20 +56,73 @@ char buf[65 * 1024];
>   }  // namespace
>   
>   int main(int argc, char** argv) {
> -  bool flush_option = false;
> -  if (argc >= 2 && strcmp(argv[1], "--flush") == 0) {
> -flush_option = true;
> ---argc;
> -++argv;
> -  }
> -  if ((argc != 2) && (argc != 1 || flush_option == false)) {
> +  struct option long_options[] = {{"max-file-size", required_argument, 0, 
> 'm'},
> +  {"max-backups", required_argument, 0, 'b'},
> +  {"flush", no_argument, 0, 'f'},
> +  {"pretty-print", required_argument, 0, 
> 'p'},
AndersW> Rename the "pretty-print" option to "print".
<> Will change it

> +  {0, 0, 0, 0}};
> +
> +  size_t maxfilesize = 0;
> +  size_t maxbackups = 0;
> +  char *pplog = NULL;
> +  int opt= 0;
> +
> +  int long_index =0;
> +  bool flush_result =  true;
> +  bool print_result =  true;
> +  bool maxfilesize_result = true;
> +  bool noof_backup_result = true;
> +  bool flush_set = false;
> +  bool prettyprint_set = false;
> +
> +  if (argc == 1) {
>   PrintUsage(argv[0]);
>   exit(EXIT_FAILURE);
> }
> -  

Re: [devel] [PATCH 0/1] Review Request for removed extra OpenSafSmfConfig in xml [#2762]

[Syam Prasad Talluri]

ACK from my Side.

Thanks,
Syam.
-Original Message-
From: Srinivas [mailto:srinivas.mangip...@oracle.com] 
Sent: Friday, January 19, 2018 2:18 PM
To: lennart.l...@ericsson.com; rafael.odza...@ericsson.com; 
ravisekhar.ko...@oracle.com; syam.tall...@oracle.com
Cc: opensaf-devel@lists.sourceforge.net; Srinivas 

Subject: [PATCH 0/1] Review Request for removed extra OpenSafSmfConfig in xml 
[#2762]

Summary: imm: removed extra OpenSafSmfConfig in xml [#2762] Review request for 
Ticket(s): 2762 Peer Reviewer(s): lennart.l...@ericsson.com , Ravi, Shyam, 
rafael.odza...@ericsson.com Pull request to: *** LIST THE PERSON WITH PUSH 
ACCESS HERE *** Affected branch(es): develop Development branch: ticket-2762 
Base revision: e1e0d2c0dc45a5ca7789f19d58dde0a41ed19354
Personal repository: git://git.code.sf.net/u/sri-mangipudy/review


Impacted area   Impact y/n

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


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

revision 9ed9e7a38e39d41d3698d5de9dbac6f07417693f
Author: Srinivas 
Date:   Fri, 19 Jan 2018 12:51:46 +0530

imm: removed extra OpenSafSmfConfig in xml [#2762]



Complete diffstat:
--
 src/smf/config/smfsv_classes.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


Testing Commands:
-
run immxml-configure, extra OpenSafSmfConfig should not be present in the 
resultant xml.

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


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


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 ~/.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

Re: [devel] [PATCH 1/1] smf: SMF created a CCB to create smfRollbackElement object, but CCB was aborted due to IMM [#2676]

[Syam Prasad Talluri]

Hi Rafael,

As you rightly pointed,  we have just published a patch by  removing the retry 
from SmfCampaignWrapup::executeCampComplete and added retires only in 
SmfImmCcbAction::execute. We have thoroughly tested it. 

Thanks,
Syam.
-Original Message-
From: Rafael Odzakow [mailto:rafael.odza...@ericsson.com] 
Sent: Monday, November 27, 2017 8:07 PM
To: Vijay Roy 
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [devel] [PATCH 1/1] smf: SMF created a CCB to create 
smfRollbackElement object, but CCB was aborted due to IMM [#2676]

SmfCampaignWrapup::executeCampComplete()  is calling SmfImmCcbAction. So looks 
like a double retry spread out across two classes. Why not have the retries in  
SmfImmCcbAction::execute only?



On 11/17/2017 07:12 AM, Vijay Roy wrote:
>
> Hi Rafael,
>
> We need the while loop in “SmfCampaignWrapup::executeCampComplete()” 
> as we encountered the issue at Wrapup too while testing.
>
> Thanks
>
> Vijay
>
> -Original Message-
> From: Rafael Odzakow [mailto:rafael.odza...@ericsson.com]
> Sent: Thursday, November 16, 2017 8:54 PM
> To: Vijay Roy 
> Cc: opensaf-devel@lists.sourceforge.net; Lennart Lund 
> 
> Subject: Re: [PATCH 1/1] smf: SMF created a CCB to create 
> smfRollbackElement object, but CCB was aborted due to IMM [#2676]
>
> What about the added while loops in
>
> SmfCampaignWrapup::executeCampComplete() should they be removed now 
> with this addition?
>
> On 11/16/2017 10:44 AM, Vijay Roy wrote:
>
> > Handling ERROR_EXIST in smfRollbackElement creation and handling
>
> > TRY_AGAIN in immCCBOperations.
>
> > ---
>
> >   src/smf/smfd/SmfUpgradeAction.cc | 26 +++---
>
> >   1 file changed, 19 insertions(+), 7 deletions(-)
>
> >
>
> > diff --git a/src/smf/smfd/SmfUpgradeAction.cc
>
> > b/src/smf/smfd/SmfUpgradeAction.cc
>
> > index 94c3dfd..af75cd7 100644
>
> > --- a/src/smf/smfd/SmfUpgradeAction.cc
>
> > +++ b/src/smf/smfd/SmfUpgradeAction.cc
>
> > @@ -28,6 +28,7 @@
>
> >   #include "smf/smfd/SmfUtils.h"
>
> >   #include "smfd.h"
>
> >   #include "smf/smfd/SmfTargetTemplate.h"
>
> > +#include "base/time.h"
>
> >
>
> >   /*
> ==
> ==
>
> >    *   DEFINITIONS
>
> > @@ -460,6 +461,7 @@ SaAisErrorT
> SmfImmCcbAction::execute(SaImmOiHandleT i_oiHandle,
>
> > const std::string* i_rollbackDn) {
>
> > SaAisErrorT result = SA_AIS_OK;
>
> > SmfRollbackCcb* rollbackCcb = NULL;
>
> > +  base::Timer doImmOpTimer(6);
>
> >
>
> > TRACE_ENTER();
>
> >
>
> > @@ -473,8 +475,8 @@ SaAisErrorT
> SmfImmCcbAction::execute(SaImmOiHandleT i_oiHandle,
>
> >   immRollbackCcbDn += ",";
>
> >   immRollbackCcbDn += *i_rollbackDn;
>
> >
>
> > -    if ((result = smfCreateRollbackElement(immRollbackCcbDn,
> i_oiHandle)) !=
>
> > -    SA_AIS_OK) {
>
> > +    result = smfCreateRollbackElement(immRollbackCcbDn, 
> > +i_oiHandle);
>
> > +    if ((result != SA_AIS_OK) && (result != SA_AIS_ERR_EXIST)) {
>
> > LOG_ER(
>
> > "SmfImmCcbAction::execute failed to create rollback element %s, 
> > rc=%s",
>
> > immRollbackCcbDn.c_str(), saf_error(result)); @@ -490,11
>
> > +492,21 @@ SaAisErrorT SmfImmCcbAction::execute(SaImmOiHandleT
> i_oiHandle,
>
> > }
>
> >
>
> > if (m_operations.size() > 0) {
>
> > -    SmfImmUtils immUtil;
>
> > -    if ((result = immUtil.doImmOperations(m_operations,
> rollbackCcb)) !=
>
> > -    SA_AIS_OK) {
>
> > -  delete rollbackCcb;
>
> > -  rollbackCcb = NULL;
>
> > +
>
> > + doImmOpTimer.set_timeout_time(6);
>
> > +    while (doImmOpTimer.is_timeout() == false) {
>
> > +  SmfImmUtils immUtil;
>
> > +  result = immUtil.doImmOperations(m_operations, rollbackCcb);
>
> > +  if (result == SA_AIS_ERR_TRY_AGAIN) {
>
> > + base::Sleep(base::kFiveHundredMilliseconds);
>
> > + continue;
>
> > +  } else if (result != SA_AIS_OK) {
>
> > + LOG_WA("%s: SmfImmCcbAction:execute Fail '%s'",
>
> > + __FUNCTION__, saf_error(result));
>
> > + delete rollbackCcb;
>
> > + rollbackCcb = NULL;
>
> > +  }
>
> > +  break;
>
> >   }
>
> > }
>
> >
>

--
Check out the vibrant tech community on one of the world's most engaging tech 
sites, Slashdot.org! 
https://urldefense.proofpoint.com/v2/url?u=http-3A__sdm.link_slashdot=DwIGaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=V-rHpog7bglMV_qtz_u-J_IeS6G1w2MSO-XRrmj38iQ=3BNorA0hrRBHey_XMs0osATb17MU9JWzr1dNYJq2XZo=idWjmHhzWdiR4C1-0qqhsTpi13go7QbkgfDi3cv9XH0=
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net