Hi Vu

I have a few comments. There are no comments inline.

1)
In several places the following (or almost the same)code can be found:

  uint32_t count = 0, stream_id = 0;
  uint32_t num = get_number_of_streams();
  stream = log_stream_get_by_id(stream_id);
  while (count < num) {
    if (stream == nullptr) goto next;

    count++;
    if (log_stream_file_close(stream) != 0)
      LOG_WA("Could not close file for stream %s", stream->name.c_str());

 next:
    stream = log_stream_get_by_id(++stream_id);
  }

The while loop could look like:
-----------------------------------------------------------------------------------------------
while (count < num) {
  if (stream == nullptr) {
    stream = log_stream_get_by_id(stream_id);
    stream_id++;
    continue;
  }

  count++;
  if (log_stream_file_close(stream) != 0)
    LOG_WA("Could not close file for stream %s", stream->name.c_str()); 
}

2)
Try to avoid more than one operation per code line. In this case separate 
increasing the stream_id and getting the stream pointer.

3)
Since this code (or almost the same) can be found in several places a common 
function could be created e.g. a function that could be used
to iterate over all existing streams and return the stream pointers

Thanks
Lennart

> -----Original Message-----
> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au]
> Sent: den 19 september 2016 11:08
> To: Lennart Lund <lennart.l...@ericsson.com>; mahesh.va...@oracle.com
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [PATCH 1 of 1] log: fix ER no stream exists in syslog [#2043]
> 
>  osaf/services/saf/logsv/lgs/lgs_amf.cc    |  27 ++++++++---
>  osaf/services/saf/logsv/lgs/lgs_config.cc |  13 +++-
>  osaf/services/saf/logsv/lgs/lgs_evt.cc    |  28 ++++++++---
>  osaf/services/saf/logsv/lgs/lgs_imm.cc    |  70 ++++++++++++++++++++++--
> ------
>  osaf/services/saf/logsv/lgs/lgs_mbcsv.cc  |  27 ++++++++---
>  osaf/services/saf/logsv/lgs/lgs_stream.cc |   6 +-
>  6 files changed, 120 insertions(+), 51 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,18 @@
> 
>  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;
> +  uint32_t num = get_number_of_streams();
> +  stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (stream == nullptr) goto next;
> +
> +    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);
> + next:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
>  }
> 
> @@ -52,7 +57,8 @@ 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 num;
> +  uint32_t count = 0, stream_id = 0;
> 
>    TRACE_ENTER2("HA ACTIVE request");
> 
> @@ -67,12 +73,17 @@ static SaAisErrorT amf_active_state_hand
> 
>    /* check existing streams */
>    num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> +  stream = log_stream_get_by_id(stream_id);
>    if (!stream)
>      LOG_ER("No streams exist!");
> -  while (stream != NULL) {
> +  while (count < num) {
> +    if (stream == nullptr) goto next;
> +
> +    count++;
>      *stream->p_fd = -1; /* First Initialize fd */
> -    stream = log_stream_get_by_id(--num);
> +
> + next:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
> 
>  done:
> 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,8 @@ 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 num;
> +  uint32_t count = 0, stream_id = 0;
> 
>    if (n > PATH_MAX) {
>      LOG_NO("verify_root_dir Fail. Path > PATH_MAX");
> @@ -471,8 +472,11 @@ 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) {
> +  stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (stream == nullptr) goto next;
> +
> +    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());
> @@ -480,7 +484,8 @@ int lgs_cfg_verify_root_dir(const std::s
>        goto done;
>      }
> 
> -    stream = log_stream_get_by_id(--num);
> + next:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
> 
>    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,13 +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);
> +    uint32_t num = get_number_of_streams();
> +    uint32_t count = 0, stream_id = 0;
> +    stream = log_stream_get_by_id(stream_id);
>      if (!stream)
>        LOG_ER("No streams exist!");
> -    while (stream != NULL) {
> +    while (count < num) {
> +      if (stream == nullptr) goto next;
> +
> +      count++;
>        *stream->p_fd = -1; /* Initialize fd */
> -      stream = log_stream_get_by_id(--num);
> +
> +   next:
> +      stream = log_stream_get_by_id(++stream_id);
>      }
>    }
> 
> @@ -800,7 +806,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, num;
>    const char *dnPrefix = "safLgStr=";
> 
>    TRACE_ENTER();
> @@ -863,15 +870,20 @@ 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) {
> +  stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (stream == nullptr) goto next;
> +
> +    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);
> +
> + next:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
> 
>    /* 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,8 @@ bool chk_filepath_stream_exist(
>    log_stream_t *i_stream = NULL;
>    std::string i_fileName;
>    std::string i_pathName;
> -  int num;
> +  uint32_t num;
> +  uint32_t count = 0, stream_id = 0;
>    bool rc = false;
> 
>    TRACE_ENTER();
> @@ -1073,8 +1074,11 @@ 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) {
> +  i_stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (i_stream == nullptr) goto next;
> +
> +    count++;
>      TRACE("Check stream \"%s\"", i_stream->name.c_str());
>      if ((i_stream->fileName == i_fileName) &&
>          (i_stream->pathName == i_pathName)) {
> @@ -1082,7 +1086,8 @@ bool chk_filepath_stream_exist(
>        break;
>      }
> 
> -    i_stream = log_stream_get_by_id(--num);
> + next:
> +    i_stream = log_stream_get_by_id(++stream_id);
>    }
> 
>    TRACE_LEAVE2("rc = %d", rc);
> @@ -1832,15 +1837,19 @@ void logRootDirectory_filemove(
>    TRACE_ENTER();
>    log_stream_t *stream;
>    std::string current_logfile;
> -  int num;
> +  uint32_t num;
> +  uint32_t count = 0, stream_id = 0;
> 
>    /* Close and rename files at current path
>     */
>    num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  while (stream != NULL) {
> +  stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (stream == nullptr) goto next;
> +
>      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 +1864,29 @@ 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);
> +
> + next:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
> 
>    /* Create new files at new path
>     */
> +  char *current_time;
> +  count = 0;
> +  stream_id = 0;
>    num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  while (stream != NULL) {
> +  stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (stream == nullptr) goto next_2;
> +
> +    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,7 +1899,9 @@ void logRootDirectory_filemove(
>       * Used if standby and configured for split file system
>       */
>      stream->stb_logFileCurrent = stream->logFileCurrent;
> -    stream = log_stream_get_by_id(--num);
> +
> + next_2:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
>    TRACE_LEAVE();
>  }
> @@ -1898,7 +1917,8 @@ void logRootDirectory_filemove(
>  void logDataGroupname_fileown(const char *new_logDataGroupname) {
>    TRACE_ENTER();
>    log_stream_t *stream;
> -  int num;
> +  uint32_t num;
> +  uint32_t count = 0, stream_id = 0;
> 
>    if (new_logDataGroupname == NULL) {
>      LOG_ER("Data group is NULL");
> @@ -1911,10 +1931,15 @@ 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) {
> +    stream = log_stream_get_by_id(stream_id);
> +    while (count < num) {
> +      if (stream == nullptr) goto next;
> +
> +      count++;
>        lgs_own_log_files_h(stream, new_logDataGroupname);
> -      stream = log_stream_get_by_id(--num);
> +
> +   next:
> +      stream = log_stream_get_by_id(++stream_id);
>      }
>    }
>    TRACE_LEAVE();
> @@ -2708,7 +2733,8 @@ SaAisErrorT lgs_imm_init_configStreams(l
>    SaImmAttrValuesT_2 **attributes;
>    int wellknownStreamId = 0;
>    int appStreamId = 3;
> -  int streamId = 0, num;
> +  uint32_t streamId = 0, num;
> +  uint32_t count = 0, stream_id = 0;
>    SaNameT objectName;
>    const char *className = "SaLogStreamConfig";
> 
> @@ -2775,8 +2801,11 @@ SaAisErrorT lgs_imm_init_configStreams(l
>    }
> 
>    num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  while (stream != NULL) {
> +  stream = log_stream_get_by_id(stream_id);
> +  while (count < num) {
> +    if (stream == nullptr) goto next;
> +
> +    count++;
>      if (cb->scAbsenceAllowed != 0) {
>        int_rc = log_stream_open_file_restore(stream);
>        if (int_rc == -1) {
> @@ -2803,7 +2832,8 @@ SaAisErrorT lgs_imm_init_configStreams(l
>        osaf_abort(0);
>      }
> 
> -    stream = log_stream_get_by_id(--num);
> + next:
> +    stream = log_stream_get_by_id(++stream_id);
>    }
> 
>  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,8 @@ 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 num;
> +  uint32_t count = 0, stream_id = 0;
> 
>    memset(&mbcsv_arg, '\0', sizeof(NCS_MBCSV_ARG));
> 
> @@ -288,8 +289,11 @@ 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 */
> +    stream = log_stream_get_by_id(stream_id);
> +    while (count < num) { /* Iterate over all streams */
> +      if (stream == nullptr) goto next;
> +
> +      count++;
>        if (ha_state == SA_AMF_HA_ACTIVE) {
>          stream->logFileCurrent = stream->stb_logFileCurrent;
>          stream->curFileSize = stream->stb_curFileSize;
> @@ -301,7 +305,8 @@ uint32_t lgs_mbcsv_change_HA_state(lgs_c
>          *stream->p_fd = -1; /* Reopen files */
>        }
> 
> -      stream = log_stream_get_by_id(--num);
> +   next:
> +      stream = log_stream_get_by_id(++stream_id);
>      }
>    }
> 
> @@ -612,7 +617,8 @@ 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 num;
> +  uint32_t count = 0, stream_id = 0;
> 
>    /* Prepare reg. structure to encode */
>    ckpt_stream_rec = static_cast<lgs_ckpt_stream_open_t
> *>(malloc(sizeof(lgs_ckpt_stream_open_t)));
> @@ -631,9 +637,12 @@ static uint32_t edu_enc_streams(lgs_cb_t
>    ncs_enc_claim_space(uba, sizeof(lgsv_ckpt_header_t));
> 
>    num = get_number_of_streams();
> -  log_stream_rec = log_stream_get_by_id(--num);
> +  log_stream_rec = log_stream_get_by_id(stream_id);
>    /* Walk through the reg list and encode record by record */
> -  while (log_stream_rec != NULL) {
> +  while (count < num) {
> +    if (log_stream_rec == nullptr) goto next;
> +
> +    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 +654,9 @@ static uint32_t edu_enc_streams(lgs_cb_t
>        return rc;
>      }
>      ++num_rec;
> -    log_stream_rec = log_stream_get_by_id(--num);
> +
> + next:
> +    log_stream_rec = log_stream_get_by_id(++stream_id);
>    }                       /* 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,7 +107,7 @@ 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;
>  }
> 

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

Reply via email to