Re: [devel] [PATCH 1/1] log: fix to remove the cfg file when log file rotation [#3045]
Hi Canh,
Ack with minor comments.
Regards, Vu
> -Original Message-
> From: Canh Van Truong
> Sent: Thursday, May 30, 2019 3:10 PM
> To: lennart.l...@ericsson.com; vu.m.ngu...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net; Canh Van Truong
>
> Subject: [PATCH 1/1] log: fix to remove the cfg file when log file
rotation
> [#3045]
>
> If the date(include year, month and day) in oldest cfg file name and the
> date of created oldest log file is different, logd cannot find out the
> oldest cfg file. The oldest cfg file is never removed when log file
rotation.
> So number of cfg file is huge after time while the log file has already
been
> rotated.
>
> Update to remove the cfg in this case.
> Also limit the number of cfg files should be removed if there are still
huge
> cfg files in disk to avoid hanging main thread
> ---
> src/log/logd/lgs_filehdl.cc | 8 ++--
> src/log/logd/lgs_stream.cc | 96
---
> --
> 2 files changed, 47 insertions(+), 57 deletions(-)
>
> diff --git a/src/log/logd/lgs_filehdl.cc b/src/log/logd/lgs_filehdl.cc
> index be2b95e1f..f31e45883 100644
> --- a/src/log/logd/lgs_filehdl.cc
> +++ b/src/log/logd/lgs_filehdl.cc
> @@ -794,10 +794,10 @@ int get_number_of_cfg_files_hdl(void *indata,
> void *outdata,
>}
> }
>
> -if ((old_ind != -1) && (cfg_old_date == log_old_date) &&
> -(cfg_old_time <= log_old_time)) {
> - TRACE_1(" (cfg_old_date:%d == log_old_date:%d) &&"
> - " (cfg_old_time:%d <= log_old_time:%d )",
> +if ((old_ind != -1) && (((cfg_old_date == log_old_date) &&
> +(cfg_old_time <= log_old_time)) || (cfg_old_date <
log_old_date))) {
[Vu] Can simplify these if by:
If (old_ind == -1) goto done_cfg_free;
bool is_cfg_outdate_1 = (cfg_old_date == log_old_date) && (cfg_old_time <=
log_old_time);
bool is_cfg_outdate_2 = (cfg_old_date < log_old_date);
if (is_cfg_outdate_1 || is_cfg_outdate_2) {
> + TRACE_1(" (cfg_old_date:%d - log_old_date:%d) -"
> + " (cfg_old_time:%d - log_old_time:%d )",
>cfg_old_date, log_old_date, cfg_old_time, log_old_time);
>TRACE_1("oldest: %s", cfg_namelist[old_ind]->d_name);
>n = snprintf(oldest_file, max_outsize, "%s/%s", path.c_str(),
> diff --git a/src/log/logd/lgs_stream.cc b/src/log/logd/lgs_stream.cc
> index d31ae170b..cb9a935e7 100644
> --- a/src/log/logd/lgs_stream.cc
> +++ b/src/log/logd/lgs_stream.cc
> @@ -270,69 +270,59 @@ static int delete_config_file(log_stream_t
> *stream) {
>
> /**
> * Remove oldest log file until there are 'maxFilesRotated' - 1 files
left
> + * The oldest cfg files are also removed
> *
> * @param stream
> - * @return -1 on error
> + * @return true/false
> */
> -static int rotate_if_needed(log_stream_t *stream) {
> +static bool rotate_if_needed(log_stream_t *stream) {
>char oldest_log_file[PATH_MAX];
>char oldest_cfg_file[PATH_MAX];
> - int rc = 0;
> - int log_file_cnt, cfg_file_cnt;
> - bool oldest_cfg = false;
> + const int max_files_rotated =
static_cast(stream->maxFilesRotated);
>
>TRACE_ENTER();
>
> - /* Rotate out log files from previous lifes */
> - if ((log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file))
==
> - -1) {
> -rc = -1;
> -goto done;
> - }
> -
> - /* Rotate out cfg files from previous lifes */
> - if (!((cfg_file_cnt = get_number_of_cfg_files_h(stream,
oldest_cfg_file)) ==
> --1)) {
> -oldest_cfg = true;
> - }
> -
> - TRACE("delete oldest_cfg_file: %s oldest_log_file %s", oldest_cfg_file,
> -oldest_log_file);
> -
> - /*
> - ** Remove until we have one less than allowed, we are just about to
> - ** create a new one again.
> - */
> - while (log_file_cnt >= static_cast(stream->maxFilesRotated)) {
> -if ((rc = file_unlink_h(oldest_log_file)) == -1) {
> - LOG_NO("Could not log delete: %s - %s", oldest_log_file,
> strerror(errno));
> - goto done;
> + // Get number of log files and the oldest log file
> + int log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file);
> + while (log_file_cnt >= max_files_rotated) {
> +TRACE("Delete oldest_log_file %s", oldest_log_file);
> +if (file_unlink_h(oldest_log_file) == -1) {
> + LOG_NO("Delete log file fail: %s - %s", oldest_log_file,
strerror(errno));
> + return false;
> }
> -
> -if (oldest_cfg == true) {
> - oldest_cfg = false;
> - if ((rc = file_unlink_h(oldest_cfg_file)) == -1) {
> -LOG_NO("Could not cfg delete: %s - %s", oldest_cfg_file,
> - strerror(errno));
> -goto done;
> - }
> -}
> -
> -if ((log_file_cnt = get_number_of_log_files_h(stream,
oldest_log_file)) ==
> --1) {
> - rc = -1;
> - goto done;
> +log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file);
> + }
> + if (log_file_cnt == -1) return false;
> +
> + // Housekeeping for cfg files
> + int number_deleted_files = 0;
> + int
Re: [devel] [PATCH 1/1] log: fix to remove the cfg file when log file rotation [#3045]
Hi Canh
Ack with some comments. I think Note 3 is the most important
Note not tested and new algorithm is not verified.
Comments:
Note 1:
In log_filehdl.cc
Sub titles are missing in function get_number_of_cfg_files_hdl()
This function is quite long and contains many code blocks (mostly if blocks)
and it's not obvious what each block is doing. Each block (where it is not
obvious) should have a comment describing what it is doing (not how it's done).
Also the updated if statement is very complicated, not easy to read. 'if'
statements like this should be avoided if possible. Can often be done by
splitting the if statement or creating a help function to handle the logic.
This function shall return true or false, have a descriptive name and if needed
a header documenting what is is doing and if the algorithm is complicated it
has to be explained.
Note 2:
In log_stream.cc
I can seet that you have added some protection if there is a huge number of cfg
files that not has been deleted. I assume this may have happened because of the
problem that is fixed with this ticket. However a cfg file can never be huge so
I assume that you don't mean huge cfg file in the following comment. You should
change to "...huge number of..."
// If there is too much cfg files that the rotation hasn't deleted them
// in previous, lgs should limit the deleting to avoid main thread is hung
// due to the deleting huge cfg files will take long times.
// The workaround here is that hard-code to delete max 100 cfg files
// in one time. Next rotation will continue to delete them
// It is useful when upgrading system and there are huge cfg files in disk
Note 3:
There should be test cases to verify the rotation mechanism including number of
cfg files, number of log files and the new option for requesting a rotation.
Thanks
Lennart
Från: Canh Van Truong
Skickat: den 30 maj 2019 10:10
Till: Lennart Lund; Vu Minh Nguyen
Kopia: opensaf-devel@lists.sourceforge.net; Canh Van Truong
Ämne: [PATCH 1/1] log: fix to remove the cfg file when log file rotation [#3045]
If the date(include year, month and day) in oldest cfg file name and the
date of created oldest log file is different, logd cannot find out the
oldest cfg file. The oldest cfg file is never removed when log file rotation.
So number of cfg file is huge after time while the log file has already been
rotated.
Update to remove the cfg in this case.
Also limit the number of cfg files should be removed if there are still huge
cfg files in disk to avoid hanging main thread
---
src/log/logd/lgs_filehdl.cc | 8 ++--
src/log/logd/lgs_stream.cc | 96 -
2 files changed, 47 insertions(+), 57 deletions(-)
diff --git a/src/log/logd/lgs_filehdl.cc b/src/log/logd/lgs_filehdl.cc
index be2b95e1f..f31e45883 100644
--- a/src/log/logd/lgs_filehdl.cc
+++ b/src/log/logd/lgs_filehdl.cc
@@ -794,10 +794,10 @@ int get_number_of_cfg_files_hdl(void *indata, void
*outdata,
}
}
-if ((old_ind != -1) && (cfg_old_date == log_old_date) &&
-(cfg_old_time <= log_old_time)) {
- TRACE_1(" (cfg_old_date:%d == log_old_date:%d) &&"
- " (cfg_old_time:%d <= log_old_time:%d )",
+if ((old_ind != -1) && (((cfg_old_date == log_old_date) &&
+(cfg_old_time <= log_old_time)) || (cfg_old_date < log_old_date))) {
+ TRACE_1(" (cfg_old_date:%d - log_old_date:%d) -"
+ " (cfg_old_time:%d - log_old_time:%d )",
cfg_old_date, log_old_date, cfg_old_time, log_old_time);
TRACE_1("oldest: %s", cfg_namelist[old_ind]->d_name);
n = snprintf(oldest_file, max_outsize, "%s/%s", path.c_str(),
diff --git a/src/log/logd/lgs_stream.cc b/src/log/logd/lgs_stream.cc
index d31ae170b..cb9a935e7 100644
--- a/src/log/logd/lgs_stream.cc
+++ b/src/log/logd/lgs_stream.cc
@@ -270,69 +270,59 @@ static int delete_config_file(log_stream_t *stream) {
/**
* Remove oldest log file until there are 'maxFilesRotated' - 1 files left
+ * The oldest cfg files are also removed
*
* @param stream
- * @return -1 on error
+ * @return true/false
*/
-static int rotate_if_needed(log_stream_t *stream) {
+static bool rotate_if_needed(log_stream_t *stream) {
char oldest_log_file[PATH_MAX];
char oldest_cfg_file[PATH_MAX];
- int rc = 0;
- int log_file_cnt, cfg_file_cnt;
- bool oldest_cfg = false;
+ const int max_files_rotated = static_cast(stream->maxFilesRotated);
TRACE_ENTER();
- /* Rotate out log files from previous lifes */
- if ((log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file)) ==
- -1) {
-rc = -1;
-goto done;
- }
-
- /* Rotate out cfg files from previous lifes */
- if (!((cfg_file_cnt = get_number_of_cfg_files_h(stream, oldest_cfg_file)) ==
--1)) {
-oldest_cfg = true;
- }
-
- TRACE("delete oldest_cfg_file: %s oldest_log_file %s", oldest_cfg_file,
-oldest_log_file);
-
- /*
- ** Remove
[devel] [PATCH 1/1] log: fix to remove the cfg file when log file rotation [#3045]
If the date(include year, month and day) in oldest cfg file name and the
date of created oldest log file is different, logd cannot find out the
oldest cfg file. The oldest cfg file is never removed when log file rotation.
So number of cfg file is huge after time while the log file has already been
rotated.
Update to remove the cfg in this case.
Also limit the number of cfg files should be removed if there are still huge
cfg files in disk to avoid hanging main thread
---
src/log/logd/lgs_filehdl.cc | 8 ++--
src/log/logd/lgs_stream.cc | 96 -
2 files changed, 47 insertions(+), 57 deletions(-)
diff --git a/src/log/logd/lgs_filehdl.cc b/src/log/logd/lgs_filehdl.cc
index be2b95e1f..f31e45883 100644
--- a/src/log/logd/lgs_filehdl.cc
+++ b/src/log/logd/lgs_filehdl.cc
@@ -794,10 +794,10 @@ int get_number_of_cfg_files_hdl(void *indata, void
*outdata,
}
}
-if ((old_ind != -1) && (cfg_old_date == log_old_date) &&
-(cfg_old_time <= log_old_time)) {
- TRACE_1(" (cfg_old_date:%d == log_old_date:%d) &&"
- " (cfg_old_time:%d <= log_old_time:%d )",
+if ((old_ind != -1) && (((cfg_old_date == log_old_date) &&
+(cfg_old_time <= log_old_time)) || (cfg_old_date < log_old_date))) {
+ TRACE_1(" (cfg_old_date:%d - log_old_date:%d) -"
+ " (cfg_old_time:%d - log_old_time:%d )",
cfg_old_date, log_old_date, cfg_old_time, log_old_time);
TRACE_1("oldest: %s", cfg_namelist[old_ind]->d_name);
n = snprintf(oldest_file, max_outsize, "%s/%s", path.c_str(),
diff --git a/src/log/logd/lgs_stream.cc b/src/log/logd/lgs_stream.cc
index d31ae170b..cb9a935e7 100644
--- a/src/log/logd/lgs_stream.cc
+++ b/src/log/logd/lgs_stream.cc
@@ -270,69 +270,59 @@ static int delete_config_file(log_stream_t *stream) {
/**
* Remove oldest log file until there are 'maxFilesRotated' - 1 files left
+ * The oldest cfg files are also removed
*
* @param stream
- * @return -1 on error
+ * @return true/false
*/
-static int rotate_if_needed(log_stream_t *stream) {
+static bool rotate_if_needed(log_stream_t *stream) {
char oldest_log_file[PATH_MAX];
char oldest_cfg_file[PATH_MAX];
- int rc = 0;
- int log_file_cnt, cfg_file_cnt;
- bool oldest_cfg = false;
+ const int max_files_rotated = static_cast(stream->maxFilesRotated);
TRACE_ENTER();
- /* Rotate out log files from previous lifes */
- if ((log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file)) ==
- -1) {
-rc = -1;
-goto done;
- }
-
- /* Rotate out cfg files from previous lifes */
- if (!((cfg_file_cnt = get_number_of_cfg_files_h(stream, oldest_cfg_file)) ==
--1)) {
-oldest_cfg = true;
- }
-
- TRACE("delete oldest_cfg_file: %s oldest_log_file %s", oldest_cfg_file,
-oldest_log_file);
-
- /*
- ** Remove until we have one less than allowed, we are just about to
- ** create a new one again.
- */
- while (log_file_cnt >= static_cast(stream->maxFilesRotated)) {
-if ((rc = file_unlink_h(oldest_log_file)) == -1) {
- LOG_NO("Could not log delete: %s - %s", oldest_log_file,
strerror(errno));
- goto done;
+ // Get number of log files and the oldest log file
+ int log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file);
+ while (log_file_cnt >= max_files_rotated) {
+TRACE("Delete oldest_log_file %s", oldest_log_file);
+if (file_unlink_h(oldest_log_file) == -1) {
+ LOG_NO("Delete log file fail: %s - %s", oldest_log_file,
strerror(errno));
+ return false;
}
-
-if (oldest_cfg == true) {
- oldest_cfg = false;
- if ((rc = file_unlink_h(oldest_cfg_file)) == -1) {
-LOG_NO("Could not cfg delete: %s - %s", oldest_cfg_file,
- strerror(errno));
-goto done;
- }
-}
-
-if ((log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file)) ==
--1) {
- rc = -1;
- goto done;
+log_file_cnt = get_number_of_log_files_h(stream, oldest_log_file);
+ }
+ if (log_file_cnt == -1) return false;
+
+ // Housekeeping for cfg files
+ int number_deleted_files = 0;
+ int cfg_file_cnt = get_number_of_cfg_files_h(stream, oldest_cfg_file);
+ while (cfg_file_cnt >= max_files_rotated) {
+TRACE("Delete oldest_cfg_file %s", oldest_cfg_file);
+if (file_unlink_h(oldest_cfg_file) == -1) {
+ LOG_NO("Delete cfg file fail: %s - %s", oldest_cfg_file,
strerror(errno));
+ return false;
}
-
-if (!((cfg_file_cnt = get_number_of_cfg_files_h(stream, oldest_cfg_file))
==
- -1)) {
- oldest_cfg = true;
+++number_deleted_files;
+cfg_file_cnt = get_number_of_cfg_files_h(stream, oldest_cfg_file);
+
+// If there is too much cfg files that the rotation hasn't deleted them
+// in previous, lgs should limit the deleting to avoid main thread is hung
+// due to the deleting huge cfg files will take long times.
+// The workaround here is that hard-code to
