Thanks Mahesh, and Lennart. I just send V3 patch.
Please have a look. Thanks.

Regards, Vu

> -----Original Message-----
> From: A V Mahesh [mailto:mahesh.va...@oracle.com]
> Sent: Thursday, September 22, 2016 11:34 AM
> To: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>;
> lennart.l...@ericsson.com
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] log: fix ER no stream exists in syslog [#2043]
> 
> Hi Vu,
> 
> Still we can convert this below  logic to a function which is use in
> multiple places
> 
> like log_stream_get_next_by_id()
> 
> =======================
> 
> /* Check existing streams */
>      uint32_t count = 0, stream_id = 0, max = 0;
>      uint32_t num = get_number_of_streams();
>      max = get_max_number_of_streams();
>      while (count < num && stream_id < max) {
>        stream = log_stream_get_by_id(stream_id++);
>        if (stream == nullptr) continue;
> 
> =================================
> 
> -AVM
> 
> 
> On 9/21/2016 3:38 PM, Vu Minh Nguyen wrote:
> >   osaf/services/saf/logsv/lgs/lgs_amf.cc    |  34 ++++++++++-----
> >   osaf/services/saf/logsv/lgs/lgs_config.cc |  12 +++--
> >   osaf/services/saf/logsv/lgs/lgs_evt.cc    |  31 +++++++++-----
> >   osaf/services/saf/logsv/lgs/lgs_imm.cc    |  63
+++++++++++++++++++-----
> ------
> >   osaf/services/saf/logsv/lgs/lgs_mbcsv.cc  |  29 +++++++++----
> >   osaf/services/saf/logsv/lgs/lgs_stream.cc |  16 ++++++-
> >   osaf/services/saf/logsv/lgs/lgs_stream.h  |   3 +-
> >   7 files changed, 123 insertions(+), 65 deletions(-)
> >
> >
> > The `number of streams` refers to total existing log streams in cluster.
> > And `stream_array` is the database holding all existing log streams.
> > When interating all log streams, logsv first started at the index
`number of
> streams`,
> > if getting NULL, logsv considered that case as `no stream`. This is
> absolutely wrong.
> >
> > This patch provides other way to iterate all log streams.
> >
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_amf.cc
> b/osaf/services/saf/logsv/lgs/lgs_amf.cc
> > --- a/osaf/services/saf/logsv/lgs/lgs_amf.cc
> > +++ b/osaf/services/saf/logsv/lgs/lgs_amf.cc
> > @@ -26,13 +26,19 @@
> >
> >   static void close_all_files() {
> >     log_stream_t *stream;
> > -  int num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  while (stream != NULL) {
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> > +
> > +  num = get_number_of_streams();
> > +  max = get_max_number_of_streams();
> > +  // Iterate all existing log streams in cluster
> > +  // the condition `stream_id < max` to avoid blocking
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> > +    count++;
> >       if (log_stream_file_close(stream) != 0)
> >         LOG_WA("Could not close file for stream %s",
stream->name.c_str());
> > -
> > -    stream = log_stream_get_by_id(--num);
> >     }
> >   }
> >
> > @@ -52,7 +58,7 @@ static void close_all_files() {
> >   static SaAisErrorT amf_active_state_handler(lgs_cb_t *cb,
SaInvocationT
> invocation) {
> >     log_stream_t *stream;
> >     SaAisErrorT error = SA_AIS_OK;
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >
> >     TRACE_ENTER2("HA ACTIVE request");
> >
> > @@ -67,13 +73,17 @@ static SaAisErrorT amf_active_state_hand
> >
> >     /* check existing streams */
> >     num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  if (!stream)
> > +  max = get_max_number_of_streams();
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> > +    count++;
> > +    *stream->p_fd = -1; /* First Initialize fd */
> > +  }
> > +
> > +  if (count == 0)
> >       LOG_ER("No streams exist!");
> > -  while (stream != NULL) {
> > -    *stream->p_fd = -1; /* First Initialize fd */
> > -    stream = log_stream_get_by_id(--num);
> > -  }
> >
> >   done:
> >     /* Update role independent of stream processing */
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_config.cc
> b/osaf/services/saf/logsv/lgs/lgs_config.cc
> > --- a/osaf/services/saf/logsv/lgs/lgs_config.cc
> > +++ b/osaf/services/saf/logsv/lgs/lgs_config.cc
> > @@ -458,7 +458,7 @@ int lgs_cfg_verify_root_dir(const std::s
> >     int rc = 0;
> >     log_stream_t *stream = NULL;
> >     size_t n = root_str_in.size();
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >
> >     if (n > PATH_MAX) {
> >       LOG_NO("verify_root_dir Fail. Path > PATH_MAX");
> > @@ -471,16 +471,18 @@ int lgs_cfg_verify_root_dir(const std::s
> >      * must not be larger than PATH_MAX.
> >      */
> >     num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  while (stream != NULL) {
> > +  max = get_max_number_of_streams();
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> > +    count++;
> >       if (lgs_is_valid_pathlength(stream->pathName, stream->fileName,
> >                                   root_str_in) == false) {
> >         TRACE("The rootPath is invalid (%s)", root_str_in.c_str());
> >         rc = -1;
> >         goto done;
> >       }
> > -
> > -    stream = log_stream_get_by_id(--num);
> >     }
> >
> >     if (lgs_path_is_writeable_dir_h(root_str_in) == false) {
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_evt.cc
> b/osaf/services/saf/logsv/lgs/lgs_evt.cc
> > --- a/osaf/services/saf/logsv/lgs/lgs_evt.cc
> > +++ b/osaf/services/saf/logsv/lgs/lgs_evt.cc
> > @@ -532,14 +532,19 @@ static uint32_t proc_rda_cb_msg(lgsv_lgs
> >       lgs_process_lga_down_list();
> >
> >       /* Check existing streams */
> > -    int num = get_number_of_streams();
> > -    stream = log_stream_get_by_id(--num);
> > -    if (!stream)
> > +    uint32_t count = 0, stream_id = 0, max = 0;
> > +    uint32_t num = get_number_of_streams();
> > +    max = get_max_number_of_streams();
> > +    while (count < num && stream_id < max) {
> > +      stream = log_stream_get_by_id(stream_id++);
> > +      if (stream == nullptr) continue;
> > +
> > +      count++;
> > +      *stream->p_fd = -1; /* Initialize fd */
> > +    }
> > +
> > +    if (count == 0)
> >         LOG_ER("No streams exist!");
> > -    while (stream != NULL) {
> > -      *stream->p_fd = -1; /* Initialize fd */
> > -      stream = log_stream_get_by_id(--num);
> > -    }
> >     }
> >
> >     TRACE_LEAVE();
> > @@ -800,7 +805,8 @@ SaAisErrorT create_new_app_stream(lgsv_s
> >     SaBoolT twelveHourModeFlag;
> >     SaUint32T logMaxLogrecsize_conf = 0;
> >     SaConstStringT str_name;
> > -  int num, err = 0;
> > +  int err = 0;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >     const char *dnPrefix = "safLgStr=";
> >
> >     TRACE_ENTER();
> > @@ -863,15 +869,18 @@ SaAisErrorT create_new_app_stream(lgsv_s
> >
> >     /* Verify that path and file are unique */
> >     num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  while (stream != NULL) {
> > +  max = get_max_number_of_streams();
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> > +    count++;
> >       if ((stream->fileName == open_sync_param->logFileName) &&
> >           (stream->pathName == open_sync_param->logFilePathName)) {
> >         TRACE("pathname already exist");
> >         rc = SA_AIS_ERR_INVALID_PARAM;
> >         goto done;
> >       }
> > -    stream = log_stream_get_by_id(--num);
> >     }
> >
> >     /* Verify that the name seems to be a DN */
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_imm.cc
> b/osaf/services/saf/logsv/lgs/lgs_imm.cc
> > --- a/osaf/services/saf/logsv/lgs/lgs_imm.cc
> > +++ b/osaf/services/saf/logsv/lgs/lgs_imm.cc
> > @@ -1022,7 +1022,7 @@ bool chk_filepath_stream_exist(
> >     log_stream_t *i_stream = NULL;
> >     std::string i_fileName;
> >     std::string i_pathName;
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >     bool rc = false;
> >
> >     TRACE_ENTER();
> > @@ -1073,16 +1073,18 @@ bool chk_filepath_stream_exist(
> >     /* Check if any stream has given filename and path */
> >     TRACE("Check if any stream has given filename and path");
> >     num = get_number_of_streams();
> > -  i_stream = log_stream_get_by_id(--num);
> > -  while (i_stream != NULL) {
> > +  max = get_max_number_of_streams();
> > +  while (count < num && stream_id < max) {
> > +    i_stream = log_stream_get_by_id(stream_id++);
> > +    if (i_stream == nullptr) continue;
> > +
> > +    count++;
> >       TRACE("Check stream \"%s\"", i_stream->name.c_str());
> >       if ((i_stream->fileName == i_fileName) &&
> >           (i_stream->pathName == i_pathName)) {
> >         rc = true;
> >         break;
> >       }
> > -
> > -    i_stream = log_stream_get_by_id(--num);
> >     }
> >
> >     TRACE_LEAVE2("rc = %d", rc);
> > @@ -1832,15 +1834,19 @@ void logRootDirectory_filemove(
> >     TRACE_ENTER();
> >     log_stream_t *stream;
> >     std::string current_logfile;
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >
> >     /* Close and rename files at current path
> >      */
> >     num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  while (stream != NULL) {
> > +  max = get_max_number_of_streams();
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> >       TRACE("Handling file %s", stream->logFileCurrent.c_str());
> >
> > +    count++;
> >       if (lgs_cb->ha_state == SA_AMF_HA_ACTIVE) {
> >         current_logfile = stream->logFileCurrent;
> >       } else {
> > @@ -1855,21 +1861,25 @@ void logRootDirectory_filemove(
> >         LOG_ER("Old log files could not be renamed and closed for
stream:
> %s",
> >                stream->name.c_str());
> >       }
> > -    stream = log_stream_get_by_id(--num);
> >     }
> >
> >     /* Create new files at new path
> >      */
> > -  num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  while (stream != NULL) {
> > +  char *current_time;
> > +  count = 0;
> > +  stream_id = 0;
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> > +    count++;
> >       if (lgs_create_config_file_h(new_logRootDirectory, stream) != 0) {
> >         LOG_ER("New config file could not be created for stream: %s",
> >                stream->name.c_str());
> >       }
> >
> >       /* Create the new log file based on updated configuration */
> > -    char *current_time = lgs_get_time(cur_time_in);
> > +    current_time = lgs_get_time(cur_time_in);
> >       stream->logFileCurrent = stream->fileName + "_" + current_time;
> >
> >       if ((*stream->p_fd = log_file_open(new_logRootDirectory,
> > @@ -1882,8 +1892,8 @@ void logRootDirectory_filemove(
> >        * Used if standby and configured for split file system
> >        */
> >       stream->stb_logFileCurrent = stream->logFileCurrent;
> > -    stream = log_stream_get_by_id(--num);
> >     }
> > +
> >     TRACE_LEAVE();
> >   }
> >
> > @@ -1898,7 +1908,7 @@ void logRootDirectory_filemove(
> >   void logDataGroupname_fileown(const char *new_logDataGroupname) {
> >     TRACE_ENTER();
> >     log_stream_t *stream;
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >
> >     if (new_logDataGroupname == NULL) {
> >       LOG_ER("Data group is NULL");
> > @@ -1911,12 +1921,16 @@ void logDataGroupname_fileown(const char
> >        * Change ownership of log files to this new group
> >        */
> >       num = get_number_of_streams();
> > -    stream = log_stream_get_by_id(--num);
> > -    while (stream != NULL) {
> > +    max = get_max_number_of_streams();
> > +    while (count < num && stream_id < max) {
> > +      stream = log_stream_get_by_id(stream_id++);
> > +      if (stream == nullptr) continue;
> > +
> > +      count++;
> >         lgs_own_log_files_h(stream, new_logDataGroupname);
> > -      stream = log_stream_get_by_id(--num);
> >       }
> >     }
> > +
> >     TRACE_LEAVE();
> >   }
> >
> > @@ -2708,7 +2722,8 @@ SaAisErrorT lgs_imm_init_configStreams(l
> >     SaImmAttrValuesT_2 **attributes;
> >     int wellknownStreamId = 0;
> >     int appStreamId = 3;
> > -  int streamId = 0, num;
> > +  uint32_t streamId = 0;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >     SaNameT objectName;
> >     const char *className = "SaLogStreamConfig";
> >
> > @@ -2775,8 +2790,12 @@ SaAisErrorT lgs_imm_init_configStreams(l
> >     }
> >
> >     num = get_number_of_streams();
> > -  stream = log_stream_get_by_id(--num);
> > -  while (stream != NULL) {
> > +  max = get_max_number_of_streams();
> > +  while (count < num && stream_id < max) {
> > +    stream = log_stream_get_by_id(stream_id++);
> > +    if (stream == nullptr) continue;
> > +
> > +    count++;
> >       if (cb->scAbsenceAllowed != 0) {
> >         int_rc = log_stream_open_file_restore(stream);
> >         if (int_rc == -1) {
> > @@ -2802,8 +2821,6 @@ SaAisErrorT lgs_imm_init_configStreams(l
> >         LOG_ER("immutil_update_one_rattr failed %s", saf_error(ais_rc));
> >         osaf_abort(0);
> >       }
> > -
> > -    stream = log_stream_get_by_id(--num);
> >     }
> >
> >   done:
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_mbcsv.cc
> b/osaf/services/saf/logsv/lgs/lgs_mbcsv.cc
> > --- a/osaf/services/saf/logsv/lgs/lgs_mbcsv.cc
> > +++ b/osaf/services/saf/logsv/lgs/lgs_mbcsv.cc
> > @@ -264,7 +264,7 @@ done:
> >   uint32_t lgs_mbcsv_change_HA_state(lgs_cb_t *cb, SaAmfHAStateT
> ha_state) {
> >     TRACE_ENTER();
> >     NCS_MBCSV_ARG mbcsv_arg;
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >
> >     memset(&mbcsv_arg, '\0', sizeof(NCS_MBCSV_ARG));
> >
> > @@ -288,8 +288,14 @@ uint32_t lgs_mbcsv_change_HA_state(lgs_c
> >     log_stream_t *stream;
> >     if (lgs_is_split_file_system()) {
> >       num = get_number_of_streams();
> > -    stream = log_stream_get_by_id(--num);
> > -    while (stream != NULL) { /* Iterate over all streams */
> > +    max = get_max_number_of_streams();
> > +    // Iterate all existing log streams in cluster
> > +    // the condition `stream_id < max` is extra protection to avoid
blocking
> > +    while (count < num && stream_id < max) {
> > +      stream = log_stream_get_by_id(stream_id++);
> > +      if (stream == nullptr) continue;
> > +
> > +      count++;
> >         if (ha_state == SA_AMF_HA_ACTIVE) {
> >           stream->logFileCurrent = stream->stb_logFileCurrent;
> >           stream->curFileSize = stream->stb_curFileSize;
> > @@ -300,8 +306,6 @@ uint32_t lgs_mbcsv_change_HA_state(lgs_c
> >           stream->stb_curFileSize = stream->curFileSize;
> >           *stream->p_fd = -1; /* Reopen files */
> >         }
> > -
> > -      stream = log_stream_get_by_id(--num);
> >       }
> >     }
> >
> > @@ -612,7 +616,7 @@ static uint32_t edu_enc_streams(lgs_cb_t
> >     uint32_t rc = NCSCC_RC_SUCCESS, num_rec = 0;
> >     uint8_t *pheader = NULL;
> >     lgsv_ckpt_header_t ckpt_hdr;
> > -  int num;
> > +  uint32_t count = 0, stream_id = 0, max = 0, num = 0;
> >
> >     /* Prepare reg. structure to encode */
> >     ckpt_stream_rec = static_cast<lgs_ckpt_stream_open_t
> *>(malloc(sizeof(lgs_ckpt_stream_open_t)));
> > @@ -630,10 +634,16 @@ static uint32_t edu_enc_streams(lgs_cb_t
> >     }
> >     ncs_enc_claim_space(uba, sizeof(lgsv_ckpt_header_t));
> >
> > +  /* Walk through the reg list and encode record by record */
> >     num = get_number_of_streams();
> > -  log_stream_rec = log_stream_get_by_id(--num);
> > -  /* Walk through the reg list and encode record by record */
> > -  while (log_stream_rec != NULL) {
> > +  max = get_max_number_of_streams();
> > +  // Iterate all existing log streams in cluster
> > +  // the condition `stream_id < max` is extra protection to avoid
blocking
> > +  while (count < num && stream_id < max) {
> > +    log_stream_rec = log_stream_get_by_id(stream_id++);
> > +    if (log_stream_rec == nullptr) continue;
> > +
> > +    count++;
> >       lgs_ckpt_stream_open_set(log_stream_rec, ckpt_stream_rec);
> >       rc = m_NCS_EDU_EXEC(&cb->edu_hdl,
> >                           edp_ed_open_stream_rec, uba, EDP_OP_TYPE_ENC,
> ckpt_stream_rec, &ederror);
> > @@ -645,7 +655,6 @@ static uint32_t edu_enc_streams(lgs_cb_t
> >         return rc;
> >       }
> >       ++num_rec;
> > -    log_stream_rec = log_stream_get_by_id(--num);
> >     }                       /* End while RegRec */
> >
> >     /* Encode RegHeader */
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_stream.cc
> b/osaf/services/saf/logsv/lgs/lgs_stream.cc
> > --- a/osaf/services/saf/logsv/lgs/lgs_stream.cc
> > +++ b/osaf/services/saf/logsv/lgs/lgs_stream.cc
> > @@ -37,9 +37,9 @@
> >
> >   static log_stream_t **stream_array;
> >   /* We have at least the 3 well known streams. */
> > -static unsigned int stream_array_size = 3;
> > +static uint32_t stream_array_size = 3;
> >   /* Current number of streams */
> > -static unsigned int numb_of_streams;
> > +static uint32_t numb_of_streams;
> >
> >   static const uint32_t kInvalidId = static_cast<uint32_t> (-1);
> >
> > @@ -107,11 +107,21 @@ done:
> >    * @param: none
> >    * @return current number of opening streams
> >    */
> > -unsigned int get_number_of_streams() {
> > +uint32_t get_number_of_streams() {
> >     return numb_of_streams;
> >   }
> >
> >   /**
> > + * Get max number of allowed log streams in cluster
> > + *
> > + * @param: none
> > + * @return the max number of log streams
> > + */
> > +uint32_t get_max_number_of_streams() {
> > +  return stream_array_size;
> > +}
> > +
> > +/**
> >    * Close with retry at EINTR
> >    *
> >    * @param fd [in]
> > diff --git a/osaf/services/saf/logsv/lgs/lgs_stream.h
> b/osaf/services/saf/logsv/lgs/lgs_stream.h
> > --- a/osaf/services/saf/logsv/lgs/lgs_stream.h
> > +++ b/osaf/services/saf/logsv/lgs/lgs_stream.h
> > @@ -130,7 +130,8 @@ extern void log_stream_print(log_stream_
> >   extern log_stream_t *log_stream_get_by_id(uint32_t id);
> >   extern bool check_max_stream();
> >   void log_free_stream_resources(log_stream_t *stream);
> > -unsigned int get_number_of_streams();
> > +uint32_t get_number_of_streams();
> > +uint32_t get_max_number_of_streams();
> >   extern log_stream_t *log_stream_get_by_name(const std::string
> &name);
> >
> >   #endif



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

Reply via email to