Hi Canh, Ack with minor comments inline.
And consider to create test cases (e.g: create 2 streams with same targets, create/modify existing stream to same target) to verify the fix. Regards, Vu > -----Original Message----- > From: Canh Van Truong [mailto:[email protected]] > Sent: Thursday, January 11, 2018 10:34 AM > To: [email protected]; [email protected]; > [email protected] > Cc: [email protected]; Canh Van Truong > <[email protected]> > Subject: [PATCH 1/1] log: fix to reject creating log streams with same file > name in same CCB [#2752] > > LOG server should not allow creating streams which have the same target > local log file > --- > src/log/logd/lgs_imm.cc | 49 +++++++++++++++++++++++++++++++++++++--- > --------- > 1 file changed, 37 insertions(+), 12 deletions(-) > > diff --git a/src/log/logd/lgs_imm.cc b/src/log/logd/lgs_imm.cc > index 2c918555b..84411fc0e 100644 > --- a/src/log/logd/lgs_imm.cc > +++ b/src/log/logd/lgs_imm.cc > @@ -82,6 +82,11 @@ static const SaImmClassNameT logConfig_str = > static const SaImmClassNameT streamConfig_str = > const_cast<SaImmClassNameT>("SaLogStreamConfig"); > > +// The list contains path-file names when validating its name in CCB > completed > +// callback. This help for log service prevents that creating any streams > +// in same CCB with duplicated log path-file name > +std::vector<std::string *> file_path_list; [Vu] 1) Use `static` if this global container only use in this file. 2) Use std::vector<std::string> could make the code sort & clean. > + > /* FUNCTIONS > * --------- > */ > @@ -1034,8 +1039,8 @@ static lgs_stream_defval_t > *get_SaLogStreamConfig_default() { > } > > /** > - * Check if a stream with the same file name and relative path already > - * exist > + * Check if file name and relative path of new stream are duplicated with > + * a stream that already exists or in same CCB > * > * @param immOiHandle > * @param fileName > @@ -1044,9 +1049,10 @@ static lgs_stream_defval_t > *get_SaLogStreamConfig_default() { > * @param operationType > * @return true if exists > */ > -bool chk_filepath_stream_exist(std::string &fileName, std::string &pathName, > - log_stream_t *stream, > - enum CcbUtilOperationType operationType) { > +static bool check_duplicated_file_path( > + std::string &fileName, std::string &pathName, > + log_stream_t *stream, > + enum CcbUtilOperationType operationType) { > log_stream_t *i_stream = NULL; > std::string i_fileName; > std::string i_pathName; > @@ -1097,9 +1103,18 @@ bool chk_filepath_stream_exist(std::string > &fileName, std::string &pathName, > osafassert(0); > } > > - /* Check if any stream has given filename and path */ > - TRACE("Check if any stream has given filename and path"); > - // Iterate all existing log streams in cluster. > + TRACE("Check if filename and pathname are duplicated"); > + > + // Check if any streams in the same CCB has given filename and pathname > + for (auto file_path : file_path_list) { [Vu] Use `for (const auto& file_path : file_path_list` for read-only `file_path` and will not create temporary objects. > + if (*file_path == (i_pathName + i_fileName)) { > + return true; > + } > + } > + std::string *file_path_name = new std::string(i_pathName + i_fileName); > + file_path_list.push_back(file_path_name); > + > + // Check if any current streams has given filename and pathname > SaBoolT endloop = SA_FALSE, jstart = SA_TRUE; > while ((i_stream = iterate_all_streams(endloop, jstart)) && !endloop) { > jstart = SA_FALSE; > @@ -1562,13 +1577,13 @@ static SaAisErrorT check_attr_validity( > goto done; > } > > - if (chk_filepath_stream_exist(i_fileName, i_pathName, stream, > - opdata->operationType)) { > + if (check_duplicated_file_path(i_fileName, i_pathName, stream, > + opdata->operationType)) { > report_oi_error(immOiHandle, opdata->ccbId, > - "Path/file %s/%s already exist", i_pathName.c_str(), > + "Path/file %s/%s is duplicated", i_pathName.c_str(), > i_fileName.c_str()); > rc = SA_AIS_ERR_BAD_OPERATION; > - TRACE("Path/file %s/%s already exist", i_pathName.c_str(), > + TRACE("Path/file %s/%s is duplicated", i_pathName.c_str(), > i_fileName.c_str()); > goto done; > } > @@ -1864,6 +1879,15 @@ static SaAisErrorT > ccbCompletedCallback(SaImmOiHandleT immOiHandle, > assert(0); > break; > } > + > + if (rc != SA_AIS_OK) break; > + } > + > + // Deleting all elements in "file_path_list" list > + for (auto it = file_path_list.begin(); it != file_path_list.end();) { > + std::string *name = *it; > + it = file_path_list.erase(it); > + delete name; [Vu] Just call file_path_list.clear() if using std::vector<std::string> ? Thanks, Vu > } > > done: > @@ -1875,6 +1899,7 @@ done: > TRACE_LEAVE2("rc = %u", cb_rc); > return cb_rc; > } > + > /** > * Set logRootDirectory to new value > * - Close all open logfiles > -- > 2.15.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
