[ABRT PATCH 1/5] Add more unittests for ignored problem error paths

[Jakub Filak]

Related to rhbz#909968 and rhbz#905412

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 tests/ignored_problems.at | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/ignored_problems.at b/tests/ignored_problems.at
index 3013507..a3d2d91 100644
--- a/tests/ignored_problems.at
+++ b/tests/ignored_problems.at
@@ -25,7 +25,7 @@ int main(void)
 {
     {
         unlink(SET_PATH);
-        ignored_problems_t *set = ignored_problems_new(xstrdup(xstrdup(SET_PATH)));
+        ignored_problems_t *set = ignored_problems_new(xstrdup(SET_PATH));
 
         ignored_problems_add(set, "foo_blah");
         assert(0 == ignored_problems_contains(set, "foo_blah") || "The set
contains invalid problem");
@@ -53,6 +53,8 @@ int main(void)
         ignored_problems_t *set = ignored_problems_new(xstrdup("/foo/blah"));
 
         ignored_problems_add(set, FIRST_DD_ID);
+        ignored_problems_remove(set, FIRST_DD_ID);
+        assert(0 == ignored_problems_contains(set, FIRST_DD_ID) || !"Invalid file
contains a problem");
 
         ignored_problems_free(set);
     }
-- 
1.8.1.4

[ABRT PATCH 2/5] Fix memory leaks in ignored problem helpers

[Jakub Filak]

Related to rhbz#909968 and rhbz#905412

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 src/lib/ignored_problems.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/lib/ignored_problems.c b/src/lib/ignored_problems.c
index f50549f..0f7fed2 100644
--- a/src/lib/ignored_problems.c
+++ b/src/lib/ignored_problems.c
@@ -128,7 +128,7 @@ void ignored_problems_add(ignored_problems_t *set, const char
*problem_id)
         VERB1 perror_msg("Can't open ignored problems '%s'"
                 " for adding problem '%s'",
                 set->ign_set_file_path, problem_id);
-        return;
+        goto ret_add_free_hashes;
     }
     /* We can add write error checks here.
      * However, what exactly can we *do* if we detect it?
@@ -137,6 +137,7 @@ void ignored_problems_add(ignored_problems_t *set, const char
*problem_id)
                               (duphash ? duphash : ""));
     fclose(fp);
 
+ ret_add_free_hashes:
     free(duphash);
     free(uuid);
 }
@@ -260,6 +261,7 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
*problem_id)
     char *duphash = dd_load_text_ext(dd, FILENAME_DUPHASH, IGN_DD_LOAD_TEXT_FLAGS);
     dd_close(dd);
 
+    bool found = false;
     FILE *fp = fopen(set->ign_set_file_path, "r");
     if (!fp)
     {
@@ -269,10 +271,9 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
*problem_id)
         VERB1 perror_msg("Can't open '%s' and determine"
                    " whether problem '%s' belongs to it",
                    set->ign_set_file_path, problem_id);
-        return false;
+        goto ret_contains_free_hashes;
     }
 
-    bool found = false;
     unsigned line_num = 0;
     while (!found)
     {
@@ -284,9 +285,11 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
*problem_id)
         free(line);
     }
 
+    fclose(fp);
+
+ ret_contains_free_hashes:
     free(duphash);
     free(uuid);
-    fclose(fp);
 
     return found;
 }
-- 
1.8.1.4

[ABRT PATCH 2/5] Fix memory leaks in ignored problem helpers

[Denys Vlasenko]

On 03/18/2013 09:47 AM, Jakub Filak wrote:
>  Related to rhbz#909968 and rhbz#905412
>  
>  Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
>  ---
>   src/lib/ignored_problems.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>  
>  diff --git a/src/lib/ignored_problems.c b/src/lib/ignored_problems.c
>  index f50549f..0f7fed2 100644
>  --- a/src/lib/ignored_problems.c
>  +++ b/src/lib/ignored_problems.c
>  @@ -128,7 +128,7 @@ void ignored_problems_add(ignored_problems_t *set, const char
> *problem_id)
>           VERB1 perror_msg("Can't open ignored problems '%s'"
>                   " for adding problem '%s'",
>                   set->ign_set_file_path, problem_id);
>  -        return;
>  +        goto ret_add_free_hashes;
>       }
>       /* We can add write error checks here.
>        * However, what exactly can we *do* if we detect it?
>  @@ -137,6 +137,7 @@ void ignored_problems_add(ignored_problems_t *set, const char
> *problem_id)
>                                 (duphash ? duphash : ""));
>       fclose(fp);
>   
>  + ret_add_free_hashes:
>       free(duphash);
>       free(uuid);
>   }
>  @@ -260,6 +261,7 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
> *problem_id)
>       char *duphash = dd_load_text_ext(dd, FILENAME_DUPHASH, IGN_DD_LOAD_TEXT_FLAGS);
>       dd_close(dd);
>   
>  +    bool found = false;
>       FILE *fp = fopen(set->ign_set_file_path, "r");
>       if (!fp)
>       {
>  @@ -269,10 +271,9 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
> *problem_id)
>           VERB1 perror_msg("Can't open '%s' and determine"
>                      " whether problem '%s' belongs to it",
>                      set->ign_set_file_path, problem_id);
>  -        return false;
>  +        goto ret_contains_free_hashes;
>       }
>   
>  -    bool found = false;
>       unsigned line_num = 0;
>       while (!found)
>       {
>  @@ -284,9 +285,11 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
> *problem_id)
>           free(line);
>       }
>   
>  +    fclose(fp);
>  +
>  + ret_contains_free_hashes:
>       free(duphash);
>       free(uuid);
>  -    fclose(fp);
>   
>       return found;
>   } 
Looks good. I pushed this change.

[ABRT PATCH 3/5] Add maintainer-check-valgrind make target

[Jakub Filak]

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 configure.ac      | 2 ++
 tests/Makefile.am | 9 +++++++++
 tests/local.at    | 3 +--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 9c6b9c7..07d2f02 100644
--- a/configure.ac
+++ b/configure.ac
@@ -170,6 +170,8 @@ AC_SUBST([O0CFLAGS], [`echo $CFLAGS   | sed 's/-O[[0-9]] *//'
| sed 's/-Werror//
 
 AC_CONFIG_HEADERS([config.h])
 
+AC_CHECK_PROGS([VALGRIND], [valgrind])
+
 AC_CONFIG_FILES([
 	Makefile
 	abrt.pc
diff --git a/tests/Makefile.am b/tests/Makefile.am
index d77aae0..9d5df41 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -45,6 +45,15 @@ atconfig: $(top_builddir)/config.status
 check-local: $(check_DATA)
 	$(SHELL) '$(TESTSUITE)' $(TESTSUITEFLAGS)
 
+.PHONY: maintainer-check-valgrind
+maintainer-check-valgrind: $(check_DATA)
+	$(MAKE) check-local \
+		PRE_AT_CHECK='$(abs_top_builddir)/libtool --mode=execute $(VALGRIND)' \
+		VALGRIND_OPTS='--tool=memcheck --leak-check=full --show-reachable=yes
--error-exitcode=1 -q'
+
+.PHONY: maintainer-check
+maintainer-check: maintainer-check-valgrind
+
 installcheck-local: $(check_DATA)
 	$(SHELL) '$(TESTSUITE)' AUTOTEST_PATH='$(bindir)' $(TESTSUITEFLAGS)
 
diff --git a/tests/local.at b/tests/local.at
index 758906d..5d97e6a 100644
--- a/tests/local.at
+++ b/tests/local.at
@@ -16,12 +16,11 @@ m4_define([AT_COMPILE],
 # Create a test named NAME by compiling and running C file with
 # contents SOURCE.  The stdout and stderr output of the C program is
 # ignored by Autotest.
-
 m4_define([AT_TESTFUN],
 [AT_SETUP([$1])
 AT_DATA([$1.c], [$2])
 AT_COMPILE([$1])
-AT_CHECK([./$1], 0, [ignore], [ignore])
+AT_CHECK([$PRE_AT_CHECK ./$1], 0, [ignore], [ignore])
 AT_CLEANUP])
 
 AT_INIT
-- 
1.8.1.4

[ABRT PATCH 3/5] Add maintainer-check-valgrind make target

[Denys Vlasenko]

On 03/18/2013 09:47 AM, Jakub Filak wrote:
>  Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
>  ---
>   configure.ac      | 2 ++
>   tests/Makefile.am | 9 +++++++++
>   tests/local.at    | 3 +--
>   3 files changed, 12 insertions(+), 2 deletions(-)
>  
>  diff --git a/configure.ac b/configure.ac
>  index 9c6b9c7..07d2f02 100644
>  --- a/configure.ac
>  +++ b/configure.ac
>  @@ -170,6 +170,8 @@ AC_SUBST([O0CFLAGS], [`echo $CFLAGS   | sed 's/-O[[0-9]] *//'
> | sed 's/-Werror//
>   
>   AC_CONFIG_HEADERS([config.h])
>   
>  +AC_CHECK_PROGS([VALGRIND], [valgrind])
>  +
>   AC_CONFIG_FILES([
>   	Makefile
>   	abrt.pc
>  diff --git a/tests/Makefile.am b/tests/Makefile.am
>  index d77aae0..9d5df41 100644
>  --- a/tests/Makefile.am
>  +++ b/tests/Makefile.am
>  @@ -45,6 +45,15 @@ atconfig: $(top_builddir)/config.status
>   check-local: $(check_DATA)
>   	$(SHELL) '$(TESTSUITE)' $(TESTSUITEFLAGS)
>   
>  +.PHONY: maintainer-check-valgrind
>  +maintainer-check-valgrind: $(check_DATA)
>  +	$(MAKE) check-local \
>  +		PRE_AT_CHECK='$(abs_top_builddir)/libtool --mode=execute $(VALGRIND)' \
>  +		VALGRIND_OPTS='--tool=memcheck --leak-check=full --show-reachable=yes
> --error-exitcode=1 -q'
>  +
>  +.PHONY: maintainer-check
>  +maintainer-check: maintainer-check-valgrind
>  +
>   installcheck-local: $(check_DATA)
>   	$(SHELL) '$(TESTSUITE)' AUTOTEST_PATH='$(bindir)' $(TESTSUITEFLAGS)
>   
>  diff --git a/tests/local.at b/tests/local.at
>  index 758906d..5d97e6a 100644
>  --- a/tests/local.at
>  +++ b/tests/local.at
>  @@ -16,12 +16,11 @@ m4_define([AT_COMPILE],
>   # Create a test named NAME by compiling and running C file with
>   # contents SOURCE.  The stdout and stderr output of the C program is
>   # ignored by Autotest.
>  -
>   m4_define([AT_TESTFUN],
>   [AT_SETUP([$1])
>   AT_DATA([$1.c], [$2])
>   AT_COMPILE([$1])
>  -AT_CHECK([./$1], 0, [ignore], [ignore])
>  +AT_CHECK([$PRE_AT_CHECK ./$1], 0, [ignore], [ignore])
>   AT_CLEANUP])
>   
>   AT_INIT 
Looks good to me.

[ABRT PATCH 4/5] Fix leaks in koops-parser unit tests

[Jakub Filak]

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 tests/GList_append.supp | 10 ++++++++++
 tests/Makefile.am       |  3 ++-
 tests/koops-parser.at   | 37 +++++++++++++++++++++++--------------
 3 files changed, 35 insertions(+), 15 deletions(-)
 create mode 100644 tests/GList_append.supp

diff --git a/tests/GList_append.supp b/tests/GList_append.supp
new file mode 100644
index 0000000..667d825
--- /dev/null
+++ b/tests/GList_append.supp
@@ -0,0 +1,10 @@
+{
+   g_list_append
+   Memcheck:Leak
+   ...
+   fun:g_list_append
+   fun:record_oops
+   fun:abrt_koops_extract_oopses
+   fun:run_test
+   fun:main
+}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9d5df41..5ef08a0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -38,6 +38,7 @@ check_DATA = atconfig atlocal $(TESTSUITE)
 DISTCLEANFILES = atconfig
 EXTRA_DIST += atlocal.in
 EXTRA_DIST += koops-test.h
+EXTRA_DIST += GList_append.supp
 
 atconfig: $(top_builddir)/config.status
 	(cd ${top_builddir} && ./config.status ${subdir}/atconfig)
@@ -49,7 +50,7 @@ check-local: $(check_DATA)
 maintainer-check-valgrind: $(check_DATA)
 	$(MAKE) check-local \
 		PRE_AT_CHECK='$(abs_top_builddir)/libtool --mode=execute $(VALGRIND)' \
-		VALGRIND_OPTS='--tool=memcheck --leak-check=full --show-reachable=yes
--error-exitcode=1 -q'
+		VALGRIND_OPTS='--tool=memcheck --leak-check=full --show-reachable=yes
--error-exitcode=1 --suppressions=$(abs_top_builddir)/tests/GList_append.supp -q'
 
 .PHONY: maintainer-check
 maintainer-check: maintainer-check-valgrind
diff --git a/tests/koops-parser.at b/tests/koops-parser.at
index 1004307..6ff72bc 100644
--- a/tests/koops-parser.at
+++ b/tests/koops-parser.at
@@ -12,21 +12,23 @@ int run_test(const struct test_struct *test)
 	FILE *fp = xfopen_ro(test->filename);
 	fprintf(stderr, "%s\t", test->filename);
 	char *line;
+	char *version;
 	while ((line = xmalloc_fgetline(fp)) != NULL)
 	{
-		char *version = koops_extract_version(line);
-		if (version && !strcmp(version, test->expected_results))
-		{
-			log("version %s", version);
-			free(version);
-			return 0;
-		}
-
+		version = koops_extract_version(line);
 		free(line);
+		if (version && !strcmp(version, test->expected_results))
+			break;
+		free(version);
+		version = NULL;
 	}
-	log("version was not found");
+	if (version)
+		log("version %s", version);
+	else
+		log("version was not found");
+	free(version);
 	fclose(fp);
-	return 1;
+	return version == NULL;
 }
 
 int main(void)
@@ -128,9 +130,11 @@ int run_test(const struct test_struct *test)
 
 	char hash_oops1[SHA1_RESULT_LEN*2 + 1];
 	koops_hash_str(hash_oops1, oops1);
+	free(oops1);
 
 	char hash_oops2[SHA1_RESULT_LEN*2 + 1];
 	koops_hash_str(hash_oops2, oops2);
+	free(oops2);
 
 	if (!strcmp(hash_oops1, hash_oops2))
 		return 0;
@@ -165,7 +169,9 @@ AT_TESTFUN([koops_remove_jiffies_timestamp],
 int run_test(const struct test_struct *test)
 {
 	char *oops_jiffies = fread_full(test->filename);
+	char *oops_jiffies_bck = oops_jiffies;
 	char *oops_no_jiffies = fread_full(test->expected_results);
+	char *oops_no_jiffies_bck = oops_no_jiffies;
 
 	if (strncmp(oops_no_jiffies, "abrt-dump-oops: Found oopses:",
 				strlen("abrt-dump-oops: Found oopses:")) == 0) {
@@ -183,12 +189,15 @@ int run_test(const struct test_struct *test)
 	GList *oops_list = NULL;
 	koops_extract_oopses(&oops_list, oops_jiffies, strlen(oops_jiffies));
 
-	if (oops_list && !strcmp((char *)oops_list->data, oops_no_jiffies))
-		return 0;
+	int result = !(oops_list && !strcmp((char *)oops_list->data,
oops_no_jiffies));
+	if (result)
+		log("'%s' \n '%s'", (char *)oops_list->data,
oops_no_jiffies);
 
-	log("'%s' \n '%s'", (char *)oops_list->data,
oops_no_jiffies);
+	g_list_free_full(oops_list, free);
+	free(oops_no_jiffies_bck);
+	free(oops_jiffies_bck);
 
-	return 1;
+	return result;
 }
 
 int main(void)
-- 
1.8.1.4

[ABRT PATCH 4/5] Fix leaks in koops-parser unit tests

[Denys Vlasenko]

On 03/18/2013 09:47 AM, Jakub Filak wrote:
>  Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
>  ---
>   tests/GList_append.supp | 10 ++++++++++
>   tests/Makefile.am       |  3 ++-
>   tests/koops-parser.at   | 37 +++++++++++++++++++++++--------------
>   3 files changed, 35 insertions(+), 15 deletions(-)
>   create mode 100644 tests/GList_append.supp
>  
>  diff --git a/tests/GList_append.supp b/tests/GList_append.supp
>  new file mode 100644
>  index 0000000..667d825
>  --- /dev/null
>  +++ b/tests/GList_append.supp
>  @@ -0,0 +1,10 @@
>  +{
>  +   g_list_append
>  +   Memcheck:Leak
>  +   ...
>  +   fun:g_list_append
>  +   fun:record_oops
>  +   fun:abrt_koops_extract_oopses
>  +   fun:run_test
>  +   fun:main
>  +}
>  diff --git a/tests/Makefile.am b/tests/Makefile.am
>  index 9d5df41..5ef08a0 100644
>  --- a/tests/Makefile.am
>  +++ b/tests/Makefile.am
>  @@ -38,6 +38,7 @@ check_DATA = atconfig atlocal $(TESTSUITE)
>   DISTCLEANFILES = atconfig
>   EXTRA_DIST += atlocal.in
>   EXTRA_DIST += koops-test.h
>  +EXTRA_DIST += GList_append.supp
>   
>   atconfig: $(top_builddir)/config.status
>   	(cd ${top_builddir} && ./config.status ${subdir}/atconfig)
>  @@ -49,7 +50,7 @@ check-local: $(check_DATA)
>   maintainer-check-valgrind: $(check_DATA)
>   	$(MAKE) check-local \
>   		PRE_AT_CHECK='$(abs_top_builddir)/libtool --mode=execute $(VALGRIND)' \
>  -		VALGRIND_OPTS='--tool=memcheck --leak-check=full --show-reachable=yes
> --error-exitcode=1 -q'
>  +		VALGRIND_OPTS='--tool=memcheck --leak-check=full --show-reachable=yes
> --error-exitcode=1 --suppressions=$(abs_top_builddir)/tests/GList_append.supp -q'
>   
>   .PHONY: maintainer-check
>   maintainer-check: maintainer-check-valgrind
>  diff --git a/tests/koops-parser.at b/tests/koops-parser.at
>  index 1004307..6ff72bc 100644
>  --- a/tests/koops-parser.at
>  +++ b/tests/koops-parser.at
>  @@ -12,21 +12,23 @@ int run_test(const struct test_struct *test)
>   	FILE *fp = xfopen_ro(test->filename);
>   	fprintf(stderr, "%s\t", test->filename);
>   	char *line;
>  +	char *version;
>   	while ((line = xmalloc_fgetline(fp)) != NULL)
>   	{
>  -		char *version = koops_extract_version(line);
>  -		if (version && !strcmp(version, test->expected_results))
>  -		{
>  -			log("version %s", version);
>  -			free(version);
>  -			return 0;
>  -		}
>  -
>  +		version = koops_extract_version(line);
>   		free(line);
>  +		if (version && !strcmp(version, test->expected_results))
>  +			break;
>  +		free(version);
>  +		version = NULL;
>   	}
>  -	log("version was not found");
>  +	if (version)
>  +		log("version %s", version);
>  +	else
>  +		log("version was not found");
>  +	free(version);
>   	fclose(fp);
>  -	return 1;
>  +	return version == NULL;
>   }
>   
>   int main(void)
>  @@ -128,9 +130,11 @@ int run_test(const struct test_struct *test)
>   
>   	char hash_oops1[SHA1_RESULT_LEN*2 + 1];
>   	koops_hash_str(hash_oops1, oops1);
>  +	free(oops1);
>   
>   	char hash_oops2[SHA1_RESULT_LEN*2 + 1];
>   	koops_hash_str(hash_oops2, oops2);
>  +	free(oops2);
>   
>   	if (!strcmp(hash_oops1, hash_oops2))
>   		return 0;
>  @@ -165,7 +169,9 @@ AT_TESTFUN([koops_remove_jiffies_timestamp],
>   int run_test(const struct test_struct *test)
>   {
>   	char *oops_jiffies = fread_full(test->filename);
>  +	char *oops_jiffies_bck = oops_jiffies;
>   	char *oops_no_jiffies = fread_full(test->expected_results);
>  +	char *oops_no_jiffies_bck = oops_no_jiffies;
>   
>   	if (strncmp(oops_no_jiffies, "abrt-dump-oops: Found oopses:",
>   				strlen("abrt-dump-oops: Found oopses:")) == 0) {
>  @@ -183,12 +189,15 @@ int run_test(const struct test_struct *test)
>   	GList *oops_list = NULL;
>   	koops_extract_oopses(&oops_list, oops_jiffies, strlen(oops_jiffies));
>   
>  -	if (oops_list && !strcmp((char *)oops_list->data, oops_no_jiffies))
>  -		return 0;
>  +	int result = !(oops_list && !strcmp((char *)oops_list->data,
> oops_no_jiffies));
>  +	if (result)
>  +		log("'%s' \n '%s'", (char *)oops_list->data,
> oops_no_jiffies);
>   
>  -	log("'%s' \n '%s'", (char *)oops_list->data,
> oops_no_jiffies);
>  +	g_list_free_full(oops_list, free);
>  +	free(oops_no_jiffies_bck);
>  +	free(oops_jiffies_bck);
>   
>  -	return 1;
>  +	return result;
>   }
>   
>   int main(void) 

I pushed all three testsuite-related patches to git

[ABRT PATCH 5/5] Get rid of 2xdd_opendir() calls in ignored problems

[Jakub Filak]

Related to rhbz#909968 and rhbz#905412

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 src/lib/ignored_problems.c | 163 +++++++++++++++++++++++++--------------------
 1 file changed, 91 insertions(+), 72 deletions(-)

diff --git a/src/lib/ignored_problems.c b/src/lib/ignored_problems.c
index 0f7fed2..925a87f 100644
--- a/src/lib/ignored_problems.c
+++ b/src/lib/ignored_problems.c
@@ -89,20 +89,41 @@ static bool ignored_problems_eq(ignored_problems_t *set,
     return false;
 }
 
-void ignored_problems_add(ignored_problems_t *set, const char *problem_id)
+static bool ignored_problems_file_contains(ignored_problems_t *set,
+        const char *problem_id, const char *uuid, const char *duphash,
+        FILE **out_fp, const char *mode)
 {
-#if 1
-    /* Hmm, do we really want to open/lock/read/unlock/close dd *twice*?
-     * (ignored_problems_contains does that once, then we do it again).
-     */
-    if (ignored_problems_contains(set, problem_id))
+    bool found = false;
+    FILE *fp = fopen(set->ign_set_file_path, mode);
+    if (!fp)
     {
-        VERB1 log("Won't add problem '%s' to ignored problems:"
-                " it is already there", problem_id);
-        return;
+        VERB1 perror_msg("Can't open ignored problems '%s' in mode
'%s'", set->ign_set_file_path, mode);
+        goto ret_contains_end;
+    }
+
+    unsigned line_num = 0;
+    while (!found)
+    {
+        char *line = xmalloc_fgetline(fp);
+        if (!line)
+            break;
+        ++line_num;
+        found = ignored_problems_eq(set, problem_id, uuid, duphash, line, line_num);
+        free(line);
     }
-#endif
 
+ ret_contains_end:
+    if (out_fp)
+        *out_fp = fp;
+    else if (fp)
+        fclose(fp);
+
+    return found;
+}
+
+
+void ignored_problems_add(ignored_problems_t *set, const char *problem_id)
+{
     struct dump_dir *dd = dd_opendir(problem_id, IGN_DD_OPEN_FLAGS);
     if (!dd)
     {
@@ -119,45 +140,43 @@ void ignored_problems_add(ignored_problems_t *set, const char
*problem_id)
     char *duphash = dd_load_text_ext(dd, FILENAME_DUPHASH, IGN_DD_LOAD_TEXT_FLAGS);
     dd_close(dd);
 
-    FILE *fp = fopen(set->ign_set_file_path, "a");
-    if (!fp)
+    VERB1 log("Going to add problem '%s' to ignored problems",
problem_id);
+
+    FILE *fp;
+    if (!ignored_problems_file_contains(set, problem_id, uuid, duphash, &fp,
"a+"))
     {
-        /* This is not a fatal problem. We are permissive because we don't want
-         * to scare users by strange error messages.
-         */
-        VERB1 perror_msg("Can't open ignored problems '%s'"
-                " for adding problem '%s'",
-                set->ign_set_file_path, problem_id);
-        goto ret_add_free_hashes;
+        if (fp)
+        {
+            /* We can add write error checks here.
+             * However, what exactly can we *do* if we detect it?
+             */
+            fprintf(fp, "%s;%s;%s\n", problem_id, (uuid ? uuid :
""),
+                                      (duphash ? duphash : ""));
+        }
+        else
+        {
+            /* This is not a fatal problem. We are permissive because we don't want
+             * to scare users by strange error messages.
+             */
+            VERB1 log("Can't add problem '%s' to ignored
problems:"
+                      " can't open the list", problem_id);
+        }
+    }
+    else
+    {
+        VERB1 log("Won't add problem '%s' to ignored problems:"
+                " it is already there", problem_id);
     }
-    /* We can add write error checks here.
-     * However, what exactly can we *do* if we detect it?
-     */
-    fprintf(fp, "%s;%s;%s\n", problem_id, (uuid ? uuid : ""),
-                              (duphash ? duphash : ""));
-    fclose(fp);
 
- ret_add_free_hashes:
+    if (fp)
+        fclose(fp);
+
     free(duphash);
     free(uuid);
 }
 
 void ignored_problems_remove(ignored_problems_t *set, const char *problem_id)
 {
-#if 1
-    /* Hmm, do we really want to open/lock/read/unlock/close dd *twice*?
-     * (ignored_problems_contains does that once, then we do it again).
-     */
-    if (!ignored_problems_contains(set, problem_id))
-    {
-        VERB1 log("Won't remove problem '%s' from ignored
problems:"
-                  " it is already removed", problem_id);
-        return;
-    }
-#endif
-
-    VERB1 log("Removing problem '%s' from ignored problems",
problem_id);
-
     char *uuid = NULL;
     char *duphash = NULL;
     struct dump_dir *dd = dd_opendir(problem_id, IGN_DD_OPEN_FLAGS);
@@ -179,17 +198,38 @@ void ignored_problems_remove(ignored_problems_t *set, const char
*problem_id)
                 " can't open the problem", problem_id);
     }
 
-    FILE *orig_fp = fopen(set->ign_set_file_path, "r");
-    if (!orig_fp)
+    VERB1 log("Going to remove problem '%s' from ignored problems",
problem_id);
+
+    FILE *orig_fp;
+    if (!ignored_problems_file_contains(set, problem_id, uuid, duphash, &orig_fp,
"r"))
     {
-        /* This is not a fatal problem. We are permissive because we don't want
-         * to scare users by strange error messages.
-         */
-        VERB1 perror_msg("Can't open '%s' for removal of problem
'%s'",
-                set->ign_set_file_path, problem_id);
+        if (orig_fp)
+        {
+            VERB1 log("Won't remove problem '%s' from ignored
problems:"
+                      " it is already removed", problem_id);
+            /* Close orig_fp here becuase it looks like much simpler than
+             * exetendig the set of goto labels at the end of this function */
+            fclose(orig_fp);
+        }
+        else
+        {
+            /* This is not a fatal problem. We are permissive because we don't want
+             * to scare users by strange error messages.
+             */
+            VERB1 log("Can't remove problem '%s' from ignored
problems:"
+                      " can't open the list", problem_id);
+
+        }
         goto ret_free_hashes;
     }
 
+    /* orig_fp must be valid here because if ignored_problems_file_contains()
+     * returned TRUE the function ensures that orig_fp is set to a valid FILE*.
+     *
+     * But the function moved the file position indicator.
+     */
+    rewind(orig_fp);
+
     char *new_tempfile_name = xasprintf("%s.XXXXXX",
set->ign_set_file_path);
     int new_tempfile_fd = mkstemp(new_tempfile_name);
     if (new_tempfile_fd < 0)
@@ -261,33 +301,12 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
*problem_id)
     char *duphash = dd_load_text_ext(dd, FILENAME_DUPHASH, IGN_DD_LOAD_TEXT_FLAGS);
     dd_close(dd);
 
-    bool found = false;
-    FILE *fp = fopen(set->ign_set_file_path, "r");
-    if (!fp)
-    {
-        /* This is not a fatal problem. We are permissive because we don't want
-         * to scare users by strange error messages.
-         */
-        VERB1 perror_msg("Can't open '%s' and determine"
-                   " whether problem '%s' belongs to it",
-                   set->ign_set_file_path, problem_id);
-        goto ret_contains_free_hashes;
-    }
-
-    unsigned line_num = 0;
-    while (!found)
-    {
-        char *line = xmalloc_fgetline(fp);
-        if (!line)
-            break;
-        ++line_num;
-        found = ignored_problems_eq(set, problem_id, uuid, duphash, line, line_num);
-        free(line);
-    }
+    VERB1 log("Going to check if problem '%s' is in ignored problems
'%s'",
+            problem_id, set->ign_set_file_path);
 
-    fclose(fp);
+    bool found = ignored_problems_file_contains(set, problem_id, uuid, duphash,
+                    /* (FILE **) */NULL, "r");
 
- ret_contains_free_hashes:
     free(duphash);
     free(uuid);
 
-- 
1.8.1.4

[ABRT PATCH 5/5] Get rid of 2xdd_opendir() calls in ignored problems

[Denys Vlasenko]

On 03/18/2013 09:47 AM, Jakub Filak wrote:
>  Related to rhbz#909968 and rhbz#905412
>  
>  Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
>  ---
>   src/lib/ignored_problems.c | 163 +++++++++++++++++++++++++--------------------
>   1 file changed, 91 insertions(+), 72 deletions(-)
>  
>  diff --git a/src/lib/ignored_problems.c b/src/lib/ignored_problems.c
>  index 0f7fed2..925a87f 100644
>  --- a/src/lib/ignored_problems.c
>  +++ b/src/lib/ignored_problems.c
>  @@ -89,20 +89,41 @@ static bool ignored_problems_eq(ignored_problems_t *set,
>       return false;
>   }
>   
>  -void ignored_problems_add(ignored_problems_t *set, const char *problem_id)
>  +static bool ignored_problems_file_contains(ignored_problems_t *set,
>  +        const char *problem_id, const char *uuid, const char *duphash,
>  +        FILE **out_fp, const char *mode)
>   {
>  -#if 1
>  -    /* Hmm, do we really want to open/lock/read/unlock/close dd *twice*?
>  -     * (ignored_problems_contains does that once, then we do it again).
>  -     */
>  -    if (ignored_problems_contains(set, problem_id))
>  +    bool found = false;
>  +    FILE *fp = fopen(set->ign_set_file_path, mode);
>  +    if (!fp)
>       {
>  -        VERB1 log("Won't add problem '%s' to ignored problems:"
>  -                " it is already there", problem_id);
>  -        return;
>  +        VERB1 perror_msg("Can't open ignored problems '%s' in mode
> '%s'", set->ign_set_file_path, mode);
>  +        goto ret_contains_end;
>  +    }
>  +
>  +    unsigned line_num = 0;
>  +    while (!found)
>  +    {
>  +        char *line = xmalloc_fgetline(fp);
>  +        if (!line)
>  +            break;
>  +        ++line_num;
>  +        found = ignored_problems_eq(set, problem_id, uuid, duphash, line, line_num);
>  +        free(line);
>       }
>  -#endif
>   
>  + ret_contains_end:
>  +    if (out_fp)
>  +        *out_fp = fp;
>  +    else if (fp)
>  +        fclose(fp);
>  +
>  +    return found;
>  +}
>  +
>  +
>  +void ignored_problems_add(ignored_problems_t *set, const char *problem_id)
>  +{
>       struct dump_dir *dd = dd_opendir(problem_id, IGN_DD_OPEN_FLAGS);
>       if (!dd)
>       {
>  @@ -119,45 +140,43 @@ void ignored_problems_add(ignored_problems_t *set, const char
> *problem_id)
>       char *duphash = dd_load_text_ext(dd, FILENAME_DUPHASH, IGN_DD_LOAD_TEXT_FLAGS);
>       dd_close(dd);
>   
>  -    FILE *fp = fopen(set->ign_set_file_path, "a");
>  -    if (!fp)
>  +    VERB1 log("Going to add problem '%s' to ignored problems",
> problem_id);
>  +
>  +    FILE *fp;
>  +    if (!ignored_problems_file_contains(set, problem_id, uuid, duphash, &fp,
> "a+"))
>       {
>  -        /* This is not a fatal problem. We are permissive because we don't want
>  -         * to scare users by strange error messages.
>  -         */
>  -        VERB1 perror_msg("Can't open ignored problems '%s'"
>  -                " for adding problem '%s'",
>  -                set->ign_set_file_path, problem_id);
>  -        goto ret_add_free_hashes;
>  +        if (fp)
>  +        {
>  +            /* We can add write error checks here.
>  +             * However, what exactly can we *do* if we detect it?
>  +             */
>  +            fprintf(fp, "%s;%s;%s\n", problem_id, (uuid ? uuid :
> ""),
>  +                                      (duphash ? duphash : ""));
>  +        }
>  +        else
>  +        {
>  +            /* This is not a fatal problem. We are permissive because we don't want
>  +             * to scare users by strange error messages.
>  +             */
>  +            VERB1 log("Can't add problem '%s' to ignored
> problems:"
>  +                      " can't open the list", problem_id);
>  +        }
>  +    }
>  +    else
>  +    {
>  +        VERB1 log("Won't add problem '%s' to ignored problems:"
>  +                " it is already there", problem_id);
>       }
>  -    /* We can add write error checks here.
>  -     * However, what exactly can we *do* if we detect it?
>  -     */
>  -    fprintf(fp, "%s;%s;%s\n", problem_id, (uuid ? uuid : ""),
>  -                              (duphash ? duphash : ""));
>  -    fclose(fp);
>   
>  - ret_add_free_hashes:
>  +    if (fp)
>  +        fclose(fp);
>  +
>       free(duphash);
>       free(uuid);
>   }
>   
>   void ignored_problems_remove(ignored_problems_t *set, const char *problem_id)
>   {
>  -#if 1
>  -    /* Hmm, do we really want to open/lock/read/unlock/close dd *twice*?
>  -     * (ignored_problems_contains does that once, then we do it again).
>  -     */
>  -    if (!ignored_problems_contains(set, problem_id))
>  -    {
>  -        VERB1 log("Won't remove problem '%s' from ignored
> problems:"
>  -                  " it is already removed", problem_id);
>  -        return;
>  -    }
>  -#endif
>  -
>  -    VERB1 log("Removing problem '%s' from ignored problems",
> problem_id);
>  -
>       char *uuid = NULL;
>       char *duphash = NULL;
>       struct dump_dir *dd = dd_opendir(problem_id, IGN_DD_OPEN_FLAGS);
>  @@ -179,17 +198,38 @@ void ignored_problems_remove(ignored_problems_t *set, const char
> *problem_id)
>                   " can't open the problem", problem_id);
>       }
>   
>  -    FILE *orig_fp = fopen(set->ign_set_file_path, "r");
>  -    if (!orig_fp)
>  +    VERB1 log("Going to remove problem '%s' from ignored problems",
> problem_id);
>  +
>  +    FILE *orig_fp;
>  +    if (!ignored_problems_file_contains(set, problem_id, uuid, duphash, &orig_fp,
> "r"))
>       {
>  -        /* This is not a fatal problem. We are permissive because we don't want
>  -         * to scare users by strange error messages.
>  -         */
>  -        VERB1 perror_msg("Can't open '%s' for removal of problem
> '%s'",
>  -                set->ign_set_file_path, problem_id);
>  +        if (orig_fp)
>  +        {
>  +            VERB1 log("Won't remove problem '%s' from ignored
> problems:"
>  +                      " it is already removed", problem_id);
>  +            /* Close orig_fp here becuase it looks like much simpler than
>  +             * exetendig the set of goto labels at the end of this function */
>  +            fclose(orig_fp);
>  +        }
>  +        else
>  +        {
>  +            /* This is not a fatal problem. We are permissive because we don't want
>  +             * to scare users by strange error messages.
>  +             */
>  +            VERB1 log("Can't remove problem '%s' from ignored
> problems:"
>  +                      " can't open the list", problem_id);
>  +
>  +        }
>           goto ret_free_hashes;
>       }
>   
>  +    /* orig_fp must be valid here because if ignored_problems_file_contains()
>  +     * returned TRUE the function ensures that orig_fp is set to a valid FILE*.
>  +     *
>  +     * But the function moved the file position indicator.
>  +     */
>  +    rewind(orig_fp);
>  +
>       char *new_tempfile_name = xasprintf("%s.XXXXXX",
> set->ign_set_file_path);
>       int new_tempfile_fd = mkstemp(new_tempfile_name);
>       if (new_tempfile_fd < 0)
>  @@ -261,33 +301,12 @@ bool ignored_problems_contains(ignored_problems_t *set, const char
> *problem_id)
>       char *duphash = dd_load_text_ext(dd, FILENAME_DUPHASH, IGN_DD_LOAD_TEXT_FLAGS);
>       dd_close(dd);
>   
>  -    bool found = false;
>  -    FILE *fp = fopen(set->ign_set_file_path, "r");
>  -    if (!fp)
>  -    {
>  -        /* This is not a fatal problem. We are permissive because we don't want
>  -         * to scare users by strange error messages.
>  -         */
>  -        VERB1 perror_msg("Can't open '%s' and determine"
>  -                   " whether problem '%s' belongs to it",
>  -                   set->ign_set_file_path, problem_id);
>  -        goto ret_contains_free_hashes;
>  -    }
>  -
>  -    unsigned line_num = 0;
>  -    while (!found)
>  -    {
>  -        char *line = xmalloc_fgetline(fp);
>  -        if (!line)
>  -            break;
>  -        ++line_num;
>  -        found = ignored_problems_eq(set, problem_id, uuid, duphash, line, line_num);
>  -        free(line);
>  -    }
>  +    VERB1 log("Going to check if problem '%s' is in ignored problems
> '%s'",
>  +            problem_id, set->ign_set_file_path);
>   
>  -    fclose(fp);
>  +    bool found = ignored_problems_file_contains(set, problem_id, uuid, duphash,
>  +                    /* (FILE **) */NULL, "r");
>   
>  - ret_contains_free_hashes:
>       free(duphash);
>       free(uuid); 
Looks good. I pushed this change to git.

[Denys Vlasenko]

On 03/18/2013 09:47 AM, Jakub Filak wrote:
>  Related to rhbz#909968 and rhbz#905412
>  
>  Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
>  ---
>   tests/ignored_problems.at | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>  
>  diff --git a/tests/ignored_problems.at b/tests/ignored_problems.at
>  index 3013507..a3d2d91 100644
>  --- a/tests/ignored_problems.at
>  +++ b/tests/ignored_problems.at
>  @@ -25,7 +25,7 @@ int main(void)
>   {
>       {
>           unlink(SET_PATH);
>  -        ignored_problems_t *set = ignored_problems_new(xstrdup(xstrdup(SET_PATH)));
>  +        ignored_problems_t *set = ignored_problems_new(xstrdup(SET_PATH));
>   
>           ignored_problems_add(set, "foo_blah");
>           assert(0 == ignored_problems_contains(set, "foo_blah") || "The
> set contains invalid problem");
>  @@ -53,6 +53,8 @@ int main(void)
>           ignored_problems_t *set = ignored_problems_new(xstrdup("/foo/blah"));
>   
>           ignored_problems_add(set, FIRST_DD_ID);
>  +        ignored_problems_remove(set, FIRST_DD_ID);
>  +        assert(0 == ignored_problems_contains(set, FIRST_DD_ID) || !"Invalid file
> contains a problem");
>   
>           ignored_problems_free(set);
>       } 
Looks good to me