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