Hi Vu,

This patch looks optimal .

ACK , not tested

-AVM


On 9/22/2016 12:47 PM, Vu Minh Nguyen wrote:
>   osaf/services/saf/logsv/lgs/lgs_amf.cc    |  29 ++++++++-------
>   osaf/services/saf/logsv/lgs/lgs_config.cc |  10 ++---
>   osaf/services/saf/logsv/lgs/lgs_evt.cc    |  27 ++++++++------
>   osaf/services/saf/logsv/lgs/lgs_imm.cc    |  56 
> ++++++++++++++----------------
>   osaf/services/saf/logsv/lgs/lgs_mbcsv.cc  |  19 ++++------
>   osaf/services/saf/logsv/lgs/lgs_stream.cc |  48 ++++++++++++++++++++++---
>   osaf/services/saf/logsv/lgs/lgs_stream.h  |   2 +-
>   7 files changed, 110 insertions(+), 81 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,13 @@
>   
>   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) {
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +
> +  // Iterate all existing log streams in cluster.
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
>       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 +52,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;
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +  uint32_t count = 0;
>   
>     TRACE_ENTER2("HA ACTIVE request");
>   
> @@ -65,15 +66,15 @@ static SaAisErrorT amf_active_state_hand
>     conf_runtime_obj_create(cb->immOiHandle);
>     lgs_start_gcfg_applier();
>   
> -  /* check existing streams */
> -  num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  if (!stream)
> +  // Iterate all existing log streams in cluster.
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
> +    *stream->p_fd = -1; /* First Initialize fd */
> +    count++;
> +  }
> +
> +  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;
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
>   
>     if (n > PATH_MAX) {
>       LOG_NO("verify_root_dir Fail. Path > PATH_MAX");
> @@ -470,17 +470,15 @@ int lgs_cfg_verify_root_dir(const std::s
>      * Make sure that the path /rootPath/streamPath/<fileName><tail>
>      * must not be larger than PATH_MAX.
>      */
> -  num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  while (stream != NULL) {
> +  // Iterate all existing log streams in cluster.
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
>       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,17 @@ 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)
> +    // Iterate all existing log streams in cluster.
> +    uint32_t count = 0;
> +    SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +    while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +      jstart = SA_FALSE;
> +      *stream->p_fd = -1; /* Initialize fd */
> +      count++;
> +    }
> +
> +    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 +803,8 @@ SaAisErrorT create_new_app_stream(lgsv_s
>     SaBoolT twelveHourModeFlag;
>     SaUint32T logMaxLogrecsize_conf = 0;
>     SaConstStringT str_name;
> -  int num, err = 0;
> +  int err = 0;
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
>     const char *dnPrefix = "safLgStr=";
>   
>     TRACE_ENTER();
> @@ -862,16 +866,15 @@ 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) {
> +  // Iterate all existing log streams in cluster.
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
>       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,6 @@ bool chk_filepath_stream_exist(
>     log_stream_t *i_stream = NULL;
>     std::string i_fileName;
>     std::string i_pathName;
> -  int num;
>     bool rc = false;
>   
>     TRACE_ENTER();
> @@ -1072,17 +1071,16 @@ 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) {
> +  // Iterate all existing log streams in cluster.
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +  while ((i_stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
>       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 +1830,14 @@ void logRootDirectory_filemove(
>     TRACE_ENTER();
>     log_stream_t *stream;
>     std::string current_logfile;
> -  int num;
> -
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
>     /* Close and rename files at current path
>      */
> -  num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  while (stream != NULL) {
> +  // Iterate all existing log streams in cluster.
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
> +
>       TRACE("Handling file %s", stream->logFileCurrent.c_str());
> -
>       if (lgs_cb->ha_state == SA_AMF_HA_ACTIVE) {
>         current_logfile = stream->logFileCurrent;
>       } else {
> @@ -1855,21 +1852,22 @@ 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;
> +  endloop = SA_FALSE;
> +  jstart = SA_TRUE;
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
>       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 +1880,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 +1896,6 @@ void logRootDirectory_filemove(
>   void logDataGroupname_fileown(const char *new_logDataGroupname) {
>     TRACE_ENTER();
>     log_stream_t *stream;
> -  int num;
>   
>     if (new_logDataGroupname == NULL) {
>       LOG_ER("Data group is NULL");
> @@ -1910,13 +1907,14 @@ void logDataGroupname_fileown(const char
>       /* Not attribute values deletion
>        * Change ownership of log files to this new group
>        */
> -    num = get_number_of_streams();
> -    stream = log_stream_get_by_id(--num);
> -    while (stream != NULL) {
> +    // Iterate all existing log streams in cluster.
> +    SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +    while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +      jstart = SA_FALSE;
>         lgs_own_log_files_h(stream, new_logDataGroupname);
> -      stream = log_stream_get_by_id(--num);
>       }
>     }
> +
>     TRACE_LEAVE();
>   }
>   
> @@ -2708,10 +2706,10 @@ SaAisErrorT lgs_imm_init_configStreams(l
>     SaImmAttrValuesT_2 **attributes;
>     int wellknownStreamId = 0;
>     int appStreamId = 3;
> -  int streamId = 0, num;
> +  uint32_t streamId = 0;
>     SaNameT objectName;
>     const char *className = "SaLogStreamConfig";
> -
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
>     TRACE_ENTER();
>   
>     om_rc = immutil_saImmOmInitialize(&omHandle, NULL, &immVersion);
> @@ -2774,9 +2772,9 @@ SaAisErrorT lgs_imm_init_configStreams(l
>       osaf_abort(0);
>     }
>   
> -  num = get_number_of_streams();
> -  stream = log_stream_get_by_id(--num);
> -  while (stream != NULL) {
> +  // Iterate all existing log streams in cluster.
> +  while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +    jstart = SA_FALSE;
>       if (cb->scAbsenceAllowed != 0) {
>         int_rc = log_stream_open_file_restore(stream);
>         if (int_rc == -1) {
> @@ -2802,8 +2800,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,6 @@ done:
>   uint32_t lgs_mbcsv_change_HA_state(lgs_cb_t *cb, SaAmfHAStateT ha_state) {
>     TRACE_ENTER();
>     NCS_MBCSV_ARG mbcsv_arg;
> -  int num;
>   
>     memset(&mbcsv_arg, '\0', sizeof(NCS_MBCSV_ARG));
>   
> @@ -287,9 +286,10 @@ 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 */
> +    // Iterate all existing log streams in cluster.
> +    SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +    while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> +      jstart = SA_FALSE;
>         if (ha_state == SA_AMF_HA_ACTIVE) {
>           stream->logFileCurrent = stream->stb_logFileCurrent;
>           stream->curFileSize = stream->stb_curFileSize;
> @@ -300,8 +300,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 +610,6 @@ 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;
>   
>     /* Prepare reg. structure to encode */
>     ckpt_stream_rec = static_cast<lgs_ckpt_stream_open_t 
> *>(malloc(sizeof(lgs_ckpt_stream_open_t)));
> @@ -630,10 +627,11 @@ 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);
>     /* Walk through the reg list and encode record by record */
> -  while (log_stream_rec != NULL) {
> +  // Iterate all existing log streams in cluster.
> +  SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> +  while ((log_stream_rec = iterate_all_streams(endloop, jstart)) && 
> !endloop) {
> +    jstart = SA_FALSE;
>       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 +643,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);
>   
> @@ -102,13 +102,47 @@ done:
>   }
>   
>   /**
> - * Get current number of openning streams
> + * Iterate all existing log streams in cluster (NOT thread safe).
> + * This function should be called within main thread.
>    *
> - * @param: none
> - * @return current number of opening streams
> + * @param jstart default is false. True if just starting the loop
> + * @out end - true if reach the end
> + * @return pointer to log stream
> + *
> + * Usage:
> + *   SaBoolT endloop = SA_FALSE, jstart = SA_TRUE;
> + *   while ((stream = iterate_all_streams(endloop, jstart)) && !endloop) {
> + *      jstart = SA_FALSE;
> + *      // Do somethings with stream
> + *   }
>    */
> -unsigned int get_number_of_streams() {
> -  return numb_of_streams;
> +log_stream_t *iterate_all_streams(SaBoolT &end, SaBoolT jstart) {
> +  static uint32_t count = 0, stream_id = 0;
> +  log_stream_t *stream = nullptr;
> +
> +  end = SA_FALSE;
> +  while (stream == nullptr) {
> +    // Begin the iteration, reset the data
> +    if (jstart == SA_TRUE) {
> +      count = 0;
> +      stream_id = 0;
> +      jstart = SA_FALSE;
> +    }
> +
> +    // Reach the end of stream_array or get all existing streams.
> +    if (stream_id >= stream_array_size || count >= numb_of_streams) {
> +      end = SA_TRUE;
> +      break;
> +    }
> +
> +    stream = log_stream_get_by_id(stream_id);
> +    stream_id++;
> +
> +    // count only if stream_id exists
> +    if (stream != nullptr) count++;
> +  }
> +
> +  return stream;
>   }
>   
>   /**
> 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,7 @@ 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();
> +log_stream_t *iterate_all_streams(SaBoolT &end, SaBoolT jstart);
>   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