Hi Canh,
OK , i missed the log_file_open() the call .
ACK from me .
-AVM
On 12/14/2016 12:46 PM, Canh Truong wrote:
> 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
