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