Hi VU,

Try**to have function **log_stream_get_next_by_id().

-AVM

On 9/21/2016 2:35 PM, Vu Minh Nguyen wrote:
> Hi Mahesh,
>
> I do think about it but not yet found out the better way.
> I can use the macro for this, somethings like
> GET {
> ......
> } NEXT;
>
> but it will violate the coding rule - avoid using the macro.
>
> I would appreciate if you have any proposal for this. Thanks.
>
> Regards, Vu
>
>> -----Original Message-----
>> From: A V Mahesh [mailto:mahesh.va...@oracle.com]
>> Sent: Wednesday, September 21, 2016 3:58 PM
>> 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,
>>
>> You used this similar code logic  number of time in multiple file in log
>> service  code
>>
>> is it possibly to optimize this logic in single function , to make
>> maintainability of code ,
>>
>> so that any bug fix will not trigger multiple places code changes
>>
>> ====================================================
>>
>> /* Verify that path and file are unique */
>>     num = get_number_of_streams();
>>     stream = log_stream_get_by_id(stream_id);
>>     while (count < num) {
>>       if (stream == nullptr) goto next
>>
>>       ............
>>
>>      next:
>>         stream = log_stream_get_by_id(++stream_id);
>>
>> ===================================================
>>
>> -AVM
>>
>>
>> On 9/19/2016 2:38 PM, Vu Minh Nguyen wrote:
>>>    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