- consider problems older than 3 days as outdated - closes trac#925
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/applet/applet.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/applet/applet.c b/src/applet/applet.c index 18d9c7b..4cfd1ea 100644 --- a/src/applet/applet.c +++ b/src/applet/applet.c @@ -1285,6 +1285,11 @@ int main(int argc, char** argv) GList *new_dirs = NULL; GList *notify_list = NULL; new_dir_exists(&new_dirs); + + /* Age limit = now - 3 days */ + char min_born_time[sizeof(time_t)*3 + 1]; + snprintf(min_born_time, sizeof(min_born_time), "%lu", (unsigned long)(time(NULL) - 3*24*60*60)); + while (new_dirs) { struct dump_dir *dd = dd_opendir((char *)new_dirs->data, DD_OPEN_READONLY); @@ -1297,7 +1302,10 @@ int main(int argc, char** argv)
char *reported_to = dd_load_text_ext(dd, FILENAME_REPORTED_TO, DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE); - if (reported_to == NULL) + char *time_str = dd_load_text_ext(dd, FILENAME_TIME, + DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE); + + if (reported_to == NULL && strcmp(time_str, min_born_time) >= 0) { problem_info_t *pi = problem_info_new(); pi->problem_dir = xstrdup((char *)new_dirs->data); @@ -1315,6 +1323,7 @@ int main(int argc, char** argv) notify_list = g_list_prepend(notify_list, pi); }
+ free(time_str); free(reported_to); dd_close(dd);
On 01/07/2013 04:42 PM, Jakub Filak wrote:
- consider problems older than 3 days as outdated
- closes trac#925
Signed-off-by: Jakub Filak jfilak@redhat.com
src/applet/applet.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/applet/applet.c b/src/applet/applet.c index 18d9c7b..4cfd1ea 100644 --- a/src/applet/applet.c +++ b/src/applet/applet.c @@ -1285,6 +1285,11 @@ int main(int argc, char** argv) GList *new_dirs = NULL; GList *notify_list = NULL; new_dir_exists(&new_dirs);
- /* Age limit = now - 3 days */
- char min_born_time[sizeof(time_t)*3 + 1];
- snprintf(min_born_time, sizeof(min_born_time), "%lu", (unsigned long)(time(NULL) - 3*24*60*60));
since you print it as a long, should use sizeof(long), not sizeof(time_t) above.
- while (new_dirs) { struct dump_dir *dd = dd_opendir((char *)new_dirs->data, DD_OPEN_READONLY);
@@ -1297,7 +1302,10 @@ int main(int argc, char** argv)
char *reported_to = dd_load_text_ext(dd, FILENAME_REPORTED_TO, DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
if (reported_to == NULL)
char *time_str = dd_load_text_ext(dd, FILENAME_TIME,DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
No, you don't want DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE -
if (reported_to == NULL && strcmp(time_str, min_born_time) >= 0)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it would go BOOM here
{ problem_info_t *pi = problem_info_new(); pi->problem_dir = xstrdup((char *)new_dirs->data);@@ -1315,6 +1323,7 @@ int main(int argc, char** argv) notify_list = g_list_prepend(notify_list, pi); }
free(time_str); free(reported_to); dd_close(dd);
On Tuesday 08 of January 2013 14:17:39 Denys Vlasenko wrote:
On 01/07/2013 04:42 PM, Jakub Filak wrote:
- consider problems older than 3 days as outdated
- closes trac#925
Signed-off-by: Jakub Filak jfilak@redhat.com
src/applet/applet.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/applet/applet.c b/src/applet/applet.c index 18d9c7b..4cfd1ea 100644 --- a/src/applet/applet.c +++ b/src/applet/applet.c @@ -1285,6 +1285,11 @@ int main(int argc, char** argv) GList *new_dirs = NULL; GList *notify_list = NULL; new_dir_exists(&new_dirs);
- /* Age limit = now - 3 days */
- char min_born_time[sizeof(time_t)*3 + 1];
- snprintf(min_born_time, sizeof(min_born_time), "%lu", (unsigned long)(time(NULL) - 3*24*60*60));
since you print it as a long, should use sizeof(long), not sizeof(time_t) above.
Good catch!
- while (new_dirs) { struct dump_dir *dd = dd_opendir((char *)new_dirs->data, DD_OPEN_READONLY);
@@ -1297,7 +1302,10 @@ int main(int argc, char** argv)
char *reported_to = dd_load_text_ext(dd, FILENAME_REPORTED_TO, DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
if (reported_to == NULL)
char *time_str = dd_load_text_ext(dd, FILENAME_TIME,DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);No, you don't want DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE -
You are right, but dd_open() fails if time element doesn't exist or if it doesn't contain valid time stamp. Therefore time_str can't be NULL. But I'll add some check to make the code more robust.
if (reported_to == NULL && strcmp(time_str, min_born_time) >= 0)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it would go BOOM here{ problem_info_t *pi = problem_info_new(); pi->problem_dir = xstrdup((char *)new_dirs->data);@@ -1315,6 +1323,7 @@ int main(int argc, char** argv) notify_list = g_list_prepend(notify_list, pi); }
free(time_str); free(reported_to); dd_close(dd);
On 01/08/2013 02:30 PM, Jakub Filak wrote:
- char *time_str = dd_load_text_ext(dd, FILENAME_TIME,
- DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
No, you don't want DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE -
You are right, but dd_open() fails if time element doesn't exist or if it doesn't contain valid time stamp. Therefore time_str can't be NULL. But I'll add some check to make the code more robust.
No need to add any checks. Just drop DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE bit, then failure in dd_load_text_ext() will return "" and strcmp() on it won't segfault.
Missing FILENAME_TIME is a pathological case; so we don't try to do a "correct" thing here (what that would be???), we only need to make sure we won't segfault.
Denys Vlasenko dvlasenk@redhat.com writes:
On 01/07/2013 04:42 PM, Jakub Filak wrote:
- consider problems older than 3 days as outdated
- closes trac#925
Signed-off-by: Jakub Filak jfilak@redhat.com
src/applet/applet.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/applet/applet.c b/src/applet/applet.c index 18d9c7b..4cfd1ea 100644 --- a/src/applet/applet.c +++ b/src/applet/applet.c @@ -1285,6 +1285,11 @@ int main(int argc, char** argv) GList *new_dirs = NULL; GList *notify_list = NULL; new_dir_exists(&new_dirs);
- /* Age limit = now - 3 days */
- char min_born_time[sizeof(time_t)*3 + 1];
- snprintf(min_born_time, sizeof(min_born_time), "%lu", (unsigned long)(time(NULL) - 3*24*60*60));
since you print it as a long, should use sizeof(long), not sizeof(time_t) above.
- while (new_dirs) { struct dump_dir *dd = dd_opendir((char *)new_dirs->data, DD_OPEN_READONLY);
@@ -1297,7 +1302,10 @@ int main(int argc, char** argv)
char *reported_to = dd_load_text_ext(dd, FILENAME_REPORTED_TO, DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
if (reported_to == NULL)
char *time_str = dd_load_text_ext(dd, FILENAME_TIME,DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);No, you don't want DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE -
if (reported_to == NULL && strcmp(time_str, min_born_time) >= 0)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it would go BOOM here
and I don't even think it's wise to compare numbers in string
{ problem_info_t *pi = problem_info_new(); pi->problem_dir = xstrdup((char *)new_dirs->data);@@ -1315,6 +1323,7 @@ int main(int argc, char** argv) notify_list = g_list_prepend(notify_list, pi); }
free(time_str); free(reported_to); dd_close(dd);
On Tuesday 08 of January 2013 14:31:25 Nikola Pajkovsky wrote:
Denys Vlasenko dvlasenk@redhat.com writes:
On 01/07/2013 04:42 PM, Jakub Filak wrote:
- consider problems older than 3 days as outdated
- closes trac#925
Signed-off-by: Jakub Filak jfilak@redhat.com
src/applet/applet.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/applet/applet.c b/src/applet/applet.c index 18d9c7b..4cfd1ea 100644 --- a/src/applet/applet.c +++ b/src/applet/applet.c @@ -1285,6 +1285,11 @@ int main(int argc, char** argv) GList *new_dirs = NULL; GList *notify_list = NULL; new_dir_exists(&new_dirs);
- /* Age limit = now - 3 days */
- char min_born_time[sizeof(time_t)*3 + 1];
- snprintf(min_born_time, sizeof(min_born_time), "%lu", (unsigned long)(time(NULL) - 3*24*60*60));
since you print it as a long, should use sizeof(long), not sizeof(time_t) above.
- while (new_dirs) { struct dump_dir *dd = dd_opendir((char *)new_dirs->data, DD_OPEN_READONLY);
@@ -1297,7 +1302,10 @@ int main(int argc, char** argv)
char *reported_to = dd_load_text_ext(dd, FILENAME_REPORTED_TO, DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);
if (reported_to == NULL)
char *time_str = dd_load_text_ext(dd, FILENAME_TIME,DD_FAIL_QUIETLY_ENOENT | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE);No, you don't want DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE -
if (reported_to == NULL && strcmp(time_str, min_born_time) >= 0)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it would go BOOM hereand I don't even think it's wise to compare numbers in string
I thought that this implementation is simpler and faster. Should I convert each time string to a number even if I can compare it as strings?
{ problem_info_t *pi = problem_info_new(); pi->problem_dir = xstrdup((char *)new_dirs->data);@@ -1315,6 +1323,7 @@ int main(int argc, char** argv) notify_list = g_list_prepend(notify_list, pi); }
free(time_str); free(reported_to); dd_close(dd);
crash-catcher@lists.fedorahosted.org
-
Denys Vlasenko -
Jakub Filak -
Nikola Pajkovsky