Hi Lennart, Thanks for your feedback. I have added PATH_MAX check as you recommended. Please get the updated code in attached file.
Regards, Vu. >-----Original Message----- >From: Lennart Lund [mailto:lennart.l...@ericsson.com] >Sent: Monday, January 25, 2016 4:00 PM >To: Vu Minh Nguyen; Anders Widell; mathi.naic...@oracle.com >Cc: opensaf-devel@lists.sourceforge.net >Subject: RE: [PATCH 1 of 1] log: Use pathconf() instead of NAME_MAX [#279] > >Hi Vu > >See my comments inline. > >Thanks >Lennart > >> -----Original Message----- >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] >> Sent: den 25 januari 2016 05:56 >> To: Lennart Lund; Anders Widell; mathi.naic...@oracle.com >> Cc: opensaf-devel@lists.sourceforge.net >> Subject: RE: [PATCH 1 of 1] log: Use pathconf() instead of NAME_MAX [#279] >> >> Hi Lennart, >> >> I am using tab for indentation, tab-width is 8 characters (C Linux coding >> rule). >> And with multiline function signatures, each new line aligns with the first >> parameter. >> I am using emacs editor with configuration mentioned in the wiki page. >> (https://sourceforge.net/p/opensaf/wiki/Emacs/) > >[Lennart] As you say your settings seems correct. I have done a quick of only >the changed code lines again and did not see this but when I did the review I >looked also at some not changed code that was affected by the changes. So >please ignore my comment but please fix if you see this in some old code. >> >> Regarding checking PATH_MAX, I think we just need to validate the length >> at the entry points (e.g: creating streams, modifying log root, etc.), to >> make sure that >> the length of "/rootpath/streampath/filename+tail" is not over PATH_MAX. >> If it passes the entry check, we do not need to check in other places. >> That is the reason why I removed all check against PATH_MAX. How about >> your >> opinion on this? > >[Lennart] You are right about that the most important check is at the "entry >points" but I strongly recommend that checks of in parameters is done also in >at least all functions with a global scope within the service (functions that are >not declared static). If the checks at entry points are very consistent and a fail >when checking in a function should never happen then the error handling in >the function may be simplified to be just an abort (and an ER log) resulting in a >coredump. This is the kind of defensive programming that always should be >used when code shall live a long time and will have several maintainer during >its life time. > >> >> For your other comments, please see my responses inline [Vu]. >> >> Regards, Vu. >> >> >-----Original Message----- >> >From: Lennart Lund [mailto:lennart.l...@ericsson.com] >> >Sent: Friday, January 22, 2016 6:44 PM >> >To: Vu Minh Nguyen; Anders Widell; mathi.naic...@oracle.com >> >Cc: opensaf-devel@lists.sourceforge.net; Lennart Lund >> >Subject: RE: [PATCH 1 of 1] log: Use pathconf() instead of NAME_MAX >> [#279] >> > >> >Hi Vu, >> > >> >Some comments: >> > >> >General: >> >Indentation is not correct in all places, please check settings in editor >> >and fix >> [Vu] Do you mean I should use spaces for indentation (google C++ style)? >> If not, could you help to show the wrong indented code line? Thanks. > >[Lennart] I think you can ignore this comment for now. I have looked through >only the changed lines once again but could not found the places with wrong >indentation. I probably saw this in some not changed code. But please fix if >you happen to see things like this. >> > >> >lgs_config.cc >> >------------- >> >***** >> >read_log_config_environ_var_2() >> >line 936-> Original logic is changed incorrectly: >> > >> > if (lgs_conf.logRootDirectory_cnfflag == LGS_CNF_DEF) { >> > /* Has not been set when reading config object >> */ >> > if ((val_str = >> getenv("LOGSV_ROOT_DIRECTORY")) != NULL) { >> > lgs_conf.logRootDirectory = >> val_str; >> > >> >This is wrong: >> > if >> (lgs_conf.logRootDirectory.size() > PATH_MAX) { >> > >> lgs_conf.logRootDirectory_cnfflag = >> >LGS_CNF_ENV; >> > } >> > >> > } else { >> > LOG_WA("No configuration data >> for logRootDirectory >> >found"); >> > } >> > } >> > >> >Change to something like...: >> > . >> > . >> > . >> > if >> (lgs_conf.logRootDirectory.size() < PATH_MAX) { >> > >> lgs_conf.logRootDirectory_cnfflag = >> >LGS_CNF_ENV; >> > } else { >> > LOG_WA("LOG root dir read from config file >> is > >> >PATH_MAX"); >> > >> lgs_conf.logRootDirectory = ""; >> > } >> > . >> > . >> > . >> > >> [Vu] Thanks. I will correct it. >> >> >Previous logic: >> > if (lgs_conf.logRootDirectory_cnfflag == LGS_CNF_DEF) { >> > /* Has not been set when reading config object >> */ >> > if ((val_str = >> getenv("LOGSV_ROOT_DIRECTORY")) != NULL) { >> > n = >> snprintf(lgs_conf.logRootDirectory, PATH_MAX, >> >"%s", val_str); >> > if (n >= PATH_MAX) { >> > /* Fail */ >> > LOG_WA("LOG root >> dir read from config file >> is >> >> PATH_MAX"); >> > >> lgs_conf.logRootDirectory[0] = '\0'; >> > } else { >> > >> lgs_conf.logRootDirectory_cnfflag = >> >LGS_CNF_ENV; >> > } >> > } else { >> > LOG_WA("No configuration data >> for logRootDirectory >> >found"); >> > } >> > } >> > >> >***** >> >lgs_path_is_writeable_dir_h() >> >Line 1263-> Check of PATH_MAX removed: >> >Since this is a global function it's better to keep the validation of the >> >in-parameter even if PATH_MAX is checked in many places and it is very >> >unlikely >> >that this check will fail. Not doing this validation in the corresponding >> _hdl >> >function is ok since the _hdl function is always used together with the _h >> >function. >> > >> >lgs_filehdl >> >----------- >> >***** >> >Path strings are handled as string types in _h function but is converted to >> >C type string when it's given to the corresponding _hdl function and in the >> _hdl >> >function it's converted back to string type again e.g. make_log_dir_...(). >> >Would it be possible to keep string type all the time? I know there may be >> >some >> >things to think about in the generic log_file_api() (lgs_file.cc) but I >> have not >> >checked what may be problematic. >> [Vu] In the generic log_file_api(), it allocates raw data by `malloc()` then >> using raw copy `memcpy`. >> If using C++ string, structures containing C++ object will be naively copied >> with memcpy, >> the C++ string object may not work properly in this case as its methods are >> not called, I think. >[Lennart] Ok. Maybe something to improve in the future then >> > >> >***** >> >own_log_files_by_group_hdl >> >The mutex lock (below) was originally done after lgs_get_data_gid() (uses >> >getgrnam()) and chown() but you have moved it before these functions are >> >called. >> >The mutex must be unlocked when any function related to the file system >> is >> >used. >> >The thread must never "hang" with the mutex locked. >> > >> >osaf_mutex_lock_ordie(&lgs_ftcom_mutex); /* LOCK after critical section >> */ >> > >> [Vu] Thanks. I will correct it. >> >> >lgs_stream.cc >> >------------- >> >***** >> >fileopen_h() >> >Line 73-> Check of PATH_MAX removed: >> >Since this is a global function it's better to keep the validation of the >> >in-parameter even if PATH_MAX is checked in many places and it is very >> >unlikely >> >that this check will fail. Not doing this validation in the corresponding >> _hdl >> >function is ok since the _hdl function is always used together with the _h >> >function. >> >Note: >> >As an example; It is ok to remove the validation of the path against >> PATH_MAX >> >in >> >log_file_open() if this check is done in fileopen_h(). >> > >> >***** >> >file_unlink_h(); See fileopen_h() >> > >> >***** >> >get_number_of_log_files_h() >> >Line 970-> Check of PATH_MAX removed: >> >There were several checks against PATH_MAX that are removed ok. >> However a >> >validation of the complete path should be done here. See comments >> above. >> > >> >lgs_util.cc >> >---------- >> >In all _h files that creates/uses a path the path should be checked against >> >PATH_MAX. The check can be removed from all _hdl functions if this is >> done >> in >> >all _h functions. See also other files where _h functions are implemented. >> > >> >Thanks >> >Lennart >> >
lgsv_279_r2.patch
Description: Binary data
------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
