Hi Mahesh,
Could you explain more clear about this?
The first ` if (*stream->p_fd == -1) {` is to avoid closing and rename log
file if file descriptor is -1.
The second is checked if open log if open log file fail.
Regards
Canh
-----Original Message-----
From: A V Mahesh [mailto:[email protected]]
Sent: Wednesday, December 14, 2016 1:45 PM
To: Canh Van Truong; [email protected]; [email protected]
Cc: [email protected]
Subject: Re: [PATCH 1 of 1] log: fix bad file discriptor error when changing
imm attributes[#2215]
Hi Canh Van,
One more point : In function log_stream_config_change() , you have
` if (*stream->p_fd == -1) {` at the top and bottom , try to optimize to ,
code looks confusing .
Normal testing done.
-AVM
On 12/14/2016 11:52 AM, A V Mahesh wrote:
> Hi ,
>
> ACK with following minor comment. NOT tested.
>
> On 12/3/2016 11:35 AM, Canh Van Truong wrote:
>> /* first time open? */
>> - if (stream->numOpeners == 0) {
>> + if (stream->numOpeners == 0 || *stream->p_fd == -1) {
> Some compilers will through error if we don't have `( )` for each
> condition
>
> -AVM
>
> On 12/3/2016 11:35 AM, Canh Van Truong wrote:
>> osaf/services/saf/logsv/lgs/lgs_imm.cc | 14 +++++-----
>> osaf/services/saf/logsv/lgs/lgs_mbcsv.cc | 2 +-
>> osaf/services/saf/logsv/lgs/lgs_stream.cc | 38
>> ++++++++++++++----------------
>> 3 files changed, 26 insertions(+), 28 deletions(-)
>>
>>
>> Issues:
>> This is happen when changing IMM attribute in logs service.
>> Following actions will be happen in IMM apply callback:
>> 1. closing log file 2. Rename cfg/log file 3. create/open new
>> cfg/log file
>>
>> In closing action(1), it need time to sync data from cache to disc
>> device before closing file. The sync data takes long time and cause
>> log service timeout.
>> With closing action timeout, we do not know that closing file is
>> successful or not.
>>
>> After closing action(1) timeout, apply callback is returned and new
>> cfg/log file are not created.
>> If closing action timeout but log file was closed successfully, 2
>> cases may be happen:
>>
>> a. Continue change this attribute with the same stream. Closing file
>> action is happen again.
>> One request is send to log file handle thread to sync data and
>> closing file with the fd that could be closed successfull from
>> previous closing file action. That causes bad discriptor error.
>>
>> b. Write log records. Because the new log file has not be created and
>> log server still write the record to file with old fd that has be
>> closed successfull from previous closing file action.
>> This also causes bad file discriptor.
>>
>>
>> Solution:
>> In step 1 above, after closing log file fail, log service can contine
>> to rename and open new file actions.
>> Convert ER to WA and add some WA.
>>
>> 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
>> @@ -1849,7 +1849,7 @@ void logRootDirectory_filemove(
>> if (log_stream_config_change(!LGS_STREAM_CREATE_FILES,
>> old_logRootDirectory, stream,
>> current_logfile,
>> cur_time_in) != 0) {
>> - LOG_ER("Old log files could not be renamed and closed for
>> stream: %s",
>> + LOG_WA("Old log files could not be renamed and closed for
>> stream: %s",
>> stream->name.c_str());
>> }
>> }
>> @@ -1865,7 +1865,7 @@ void logRootDirectory_filemove(
>> while ((stream = iterate_all_streams(endloop, jstart)) &&
>> !endloop) {
>> jstart = SA_FALSE;
>> if (lgs_create_config_file_h(new_logRootDirectory, stream) != 0) {
>> - LOG_ER("New config file could not be created for stream: %s",
>> + LOG_WA("New config file could not be created for stream: %s",
>> stream->name.c_str());
>> }
>> @@ -1875,7 +1875,7 @@ void logRootDirectory_filemove(
>> if ((*stream->p_fd = log_file_open(new_logRootDirectory,
>> stream,
>> stream->logFileCurrent, NULL)) == -1) {
>> - LOG_ER("New log file could not be created for stream: %s",
>> + LOG_WA("New log file could not be created for stream: %s",
>> stream->name.c_str());
>> }
>> @@ -2325,13 +2325,13 @@ static void stream_ccb_apply_modify(cons
>> if ((rc = log_stream_config_change(!LGS_STREAM_CREATE_FILES,
>> root_path, stream,
>> current_logfile_name, &cur_time))
>> != 0) {
>> - LOG_ER("log_stream_config_change failed: %d", rc);
>> + LOG_WA("log_stream_config_change failed: %d", rc);
>> }
>> stream->fileName = fileName;
>> if ((rc = lgs_create_config_file_h(root_path, stream)) != 0) {
>> - LOG_ER("lgs_create_config_file_h failed: %d", rc);
>> + LOG_WA("lgs_create_config_file_h failed: %d", rc);
>> }
>> char *current_time = lgs_get_time(&cur_time);
>> @@ -2340,7 +2340,7 @@ static void stream_ccb_apply_modify(cons
>> // Create the new log file based on updated configuration
>> if ((*stream->p_fd = log_file_open(root_path,
>> stream,
>> stream->logFileCurrent, NULL)) == -1)
>> - LOG_ER("New log file could not be created for stream: %s",
>> + LOG_WA("New log file could not be created for stream: %s",
>> stream->name.c_str());
>> } else if (new_cfg_file_needed) {
>> int rc;
>> @@ -2348,7 +2348,7 @@ static void stream_ccb_apply_modify(cons
>> root_path, stream,
>> current_logfile_name,
>> &cur_time))
>> != 0) {
>> - LOG_ER("log_stream_config_change failed: %d", rc);
>> + LOG_WA("log_stream_config_change failed: %d", rc);
>> }
>> }
>> 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
>> @@ -2128,7 +2128,7 @@ static uint32_t ckpt_proc_cfg_stream(lgs
>> if ((rc = log_stream_config_change(LGS_STREAM_CREATE_FILES,
>> root_path, stream,
>> stream->stb_logFileCurrent, &closetime)) != 0) {
>> - LOG_ER("log_stream_config_change failed: %d", rc);
>> + LOG_WA("log_stream_config_change failed: %d", rc);
>> }
>> /* When modifying old files are closed and new are opened
>> meaning that
>> 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
>> @@ -729,11 +729,11 @@ int log_file_open(
>> * @param stream
>> */
>> void log_stream_open_fileinit(log_stream_t *stream) {
>> + osafassert(stream != NULL);
>> TRACE_ENTER2("%s, numOpeners=%u", stream->name.c_str(),
>> stream->numOpeners);
>> - osafassert(stream != NULL);
>> /* first time open? */
>> - if (stream->numOpeners == 0) {
>> + if (stream->numOpeners == 0 || *stream->p_fd == -1) {
>> /* Create and save current log file name */
>> stream->logFileCurrent = stream->fileName + "_" +
>> lgs_get_time(NULL);
>> log_initiate_stream_files(stream);
>> @@ -1415,7 +1415,7 @@ int log_stream_config_change(bool create
>> log_stream_t *stream,
>> const std::string ¤t_logfile_name,
>> time_t *cur_time_in) {
>> - int rc;
>> + int rc, ret = 0;
>> int errno_ret;
>> char *current_time = lgs_get_time(cur_time_in);
>> std::string emptyStr = "";
>> @@ -1433,28 +1433,31 @@ int log_stream_config_change(bool create
>> /* close the existing log file, and only when there is a valid
>> fd */
>> if ((rc = fileclose_h(*stream->p_fd, &errno_ret)) == -1) {
>> - LOG_NO("log_stream log file close FAILED: %s",
>> strerror(errno_ret));
>> - goto done;
>> + LOG_WA("log_stream log file close FAILED: %s",
>> strerror(errno_ret));
>> + ret = -1;
>> }
>> *stream->p_fd = -1;
>> rc = lgs_file_rename_h(root_path, stream->pathName,
>> current_logfile_name,
>> current_time, LGS_LOG_FILE_EXT, emptyStr);
>> if (rc == -1) {
>> - goto done;
>> + LOG_WA("log file (%s) is renamed FAILED: %d",
>> current_logfile_name.c_str(), rc);
>> }
>> rc = lgs_file_rename_h(root_path, stream->pathName,
>> stream->fileName,
>> current_time, LGS_LOG_FILE_CONFIG_EXT,
>> emptyStr);
>> if (rc == -1) {
>> - goto done;
>> + LOG_WA("cfg file (%s) is renamed FAILED: %d",
>> stream->fileName.c_str(), rc);
>> + ret = -1;
>> }
>> }
>> /* Creating the new config file */
>> if (create_files_f == LGS_STREAM_CREATE_FILES) {
>> - if ((rc = lgs_create_config_file_h(root_path, stream)) != 0)
>> - goto done;
>> + if ((rc = lgs_create_config_file_h(root_path, stream)) != 0) {
>> + LOG_WA("lgs_create_config_file_h (%s) failed: %d",
>> stream->fileName.c_str(), rc);
>> + ret = -1;
>> + }
>> stream->logFileCurrent = stream->fileName + "_" + current_time;
>> @@ -1463,20 +1466,15 @@ int log_stream_config_change(bool create
>> stream,
>> stream->logFileCurrent,
>> NULL);
>> + if (*stream->p_fd == -1) {
>> + LOG_WA("New log file could not be created for stream: %s",
>> + stream->name.c_str());
>> + ret = -1;
>> + }
>> }
>> - /* Fix bug - this function makes return (-1) when create_files_f
>> = false */
>> - if (create_files_f == !LGS_STREAM_CREATE_FILES) {
>> - rc = 0;
>> - } else if (*stream->p_fd == -1) {
>> - rc = -1;
>> - } else {
>> - rc = 0;
>> - }
>> -
>> -done:
>> TRACE_LEAVE();
>> - return rc;
>> + return ret;
>> }
>> /*
>
------------------------------------------------------------------------------
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
