[LIBREPORT PATCH] Replace %reporter thing in BZ reporter with "libreport_version" element

[Denys Vlasenko]

Rationale for %reporter was that created BZ's need information
which version of libreport was creating them.
But the same thing will be needed by other users of our data.
Adding it only in BZ reporter is a wrong place.

We already create "abrt_version" element, I copied the logic
and this change creates similar "libreport_version" element.

BZ reporter will show it along with other one-liners.

Signed-off-by: Denys Vlasenko <dvlasenk-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
src/include/internal_libreport.h | 1 +
src/lib/dump_dir.c | 2 ++
src/plugins/bugzilla_format.conf | 1 -
src/plugins/bugzilla_format_kernel.conf | 2 +-
src/plugins/bugzilla_format_libreport.conf | 1 -
src/plugins/bugzilla_formatdup.conf | 1 -
src/plugins/reporter-bugzilla.c | 4 ----
7 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h
index ae46ad4..6076823 100644
--- a/src/include/internal_libreport.h
+++ b/src/include/internal_libreport.h
@@ -861,6 +861,7 @@ struct dump_dir *open_directory_for_writing(
#define FILENAME_PKG_ARCH "pkg_arch"
#define FILENAME_USERNAME "username"
#define FILENAME_ABRT_VERSION "abrt_version"
+#define FILENAME_LIBREPORT_VERSION "libreport_version"

// Not stored as files, added "on the fly":
#define CD_DUMPDIR "Directory"
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
index 4bb23ba..9ff51b4 100644
--- a/src/lib/dump_dir.c
+++ b/src/lib/dump_dir.c
@@ -629,6 +629,8 @@ void dd_create_basic_files(struct dump_dir *dd, uid_t uid, const char *chroot_di
}
free(time_str);

+ dd_save_text(dd, FILENAME_LIBREPORT_VERSION, PACKAGE"-"VERSION);
+
/* it doesn't make sense to create the uid file if uid == -1 */
if (uid != (uid_t)-1L)
{
diff --git a/src/plugins/bugzilla_format.conf b/src/plugins/bugzilla_format.conf
index cad5876..f928799 100644
--- a/src/plugins/bugzilla_format.conf
+++ b/src/plugins/bugzilla_format.conf
@@ -48,7 +48,6 @@ Additional info:: \
-analyzer,-count,-duphash,-uuid,-abrt_version,\
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
- %reporter,\
%oneline

Truncated backtrace:: %bare_%short_backtrace
diff --git a/src/plugins/bugzilla_format_kernel.conf b/src/plugins/bugzilla_format_kernel.conf
index e46af89..c32e424 100644
--- a/src/plugins/bugzilla_format_kernel.conf
+++ b/src/plugins/bugzilla_format_kernel.conf
@@ -40,6 +40,6 @@

Description of problem:: %bare_comment

-Additional info:: %reporter, %bare_backtrace
+Additional info:: %bare_backtrace

%attach:: dmesg
diff --git a/src/plugins/bugzilla_format_libreport.conf b/src/plugins/bugzilla_format_libreport.conf
index 2ecdf6f..352be98 100644
--- a/src/plugins/bugzilla_format_libreport.conf
+++ b/src/plugins/bugzilla_format_libreport.conf
@@ -50,7 +50,6 @@ Additional info:: \
-analyzer,-count,-duphash,-uuid,-abrt_version,\
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
- %reporter,\
%oneline

Truncated backtrace:: %bare_%short_backtrace
diff --git a/src/plugins/bugzilla_formatdup.conf b/src/plugins/bugzilla_formatdup.conf
index f5e89f7..6f51606 100644
--- a/src/plugins/bugzilla_formatdup.conf
+++ b/src/plugins/bugzilla_formatdup.conf
@@ -49,5 +49,4 @@
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
-var_log_messages,\
- %reporter,\
%oneline
diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c
index 05439cd..cd3cf15 100644
--- a/src/plugins/reporter-bugzilla.c
+++ b/src/plugins/reporter-bugzilla.c
@@ -405,10 +405,6 @@ int append_item(struct strbuf *result, const char *item_name, problem_data_t *pd
if (strcmp(item_name, "%short_backtrace") == 0)
return append_short_backtrace(result, pd, CD_TEXT_ATT_SIZE_BZ, print_item_name);

- /* Compat with previously-existed ad-hockery: %reporter */
- if (strcmp(item_name, "%reporter") == 0)
- return append_text(result, "reporter", PACKAGE"-"VERSION, print_item_name);
-
/* %oneline,%multiline,%text */
bool oneline = (strcmp(item_name+1, "oneline" ) == 0);
bool multiline = (strcmp(item_name+1, "multiline") == 0);

[Jiri Moskovcak]

Have you tried to create a bz with your patch, I'd like to take a look
at the results.

Thanks,
Jirka

Post by Denys Vlasenko
Rationale for %reporter was that created BZ's need information
which version of libreport was creating them.
But the same thing will be needed by other users of our data.
Adding it only in BZ reporter is a wrong place.
We already create "abrt_version" element, I copied the logic
and this change creates similar "libreport_version" element.
BZ reporter will show it along with other one-liners.
---
src/include/internal_libreport.h | 1 +
src/lib/dump_dir.c | 2 ++
src/plugins/bugzilla_format.conf | 1 -
src/plugins/bugzilla_format_kernel.conf | 2 +-
src/plugins/bugzilla_format_libreport.conf | 1 -
src/plugins/bugzilla_formatdup.conf | 1 -
src/plugins/reporter-bugzilla.c | 4 ----
7 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h
index ae46ad4..6076823 100644
--- a/src/include/internal_libreport.h
+++ b/src/include/internal_libreport.h
@@ -861,6 +861,7 @@ struct dump_dir *open_directory_for_writing(
#define FILENAME_PKG_ARCH "pkg_arch"
#define FILENAME_USERNAME "username"
#define FILENAME_ABRT_VERSION "abrt_version"
+#define FILENAME_LIBREPORT_VERSION "libreport_version"
#define CD_DUMPDIR "Directory"
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
index 4bb23ba..9ff51b4 100644
--- a/src/lib/dump_dir.c
+++ b/src/lib/dump_dir.c
@@ -629,6 +629,8 @@ void dd_create_basic_files(struct dump_dir *dd, uid_t uid, const char *chroot_di
}
free(time_str);
+ dd_save_text(dd, FILENAME_LIBREPORT_VERSION, PACKAGE"-"VERSION);
+
/* it doesn't make sense to create the uid file if uid == -1 */
if (uid != (uid_t)-1L)
{
diff --git a/src/plugins/bugzilla_format.conf b/src/plugins/bugzilla_format.conf
index cad5876..f928799 100644
--- a/src/plugins/bugzilla_format.conf
+++ b/src/plugins/bugzilla_format.conf
@@ -48,7 +48,6 @@ Additional info:: \
-analyzer,-count,-duphash,-uuid,-abrt_version,\
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
- %reporter,\
%oneline
Truncated backtrace:: %bare_%short_backtrace
diff --git a/src/plugins/bugzilla_format_kernel.conf b/src/plugins/bugzilla_format_kernel.conf
index e46af89..c32e424 100644
--- a/src/plugins/bugzilla_format_kernel.conf
+++ b/src/plugins/bugzilla_format_kernel.conf
@@ -40,6 +40,6 @@
Description of problem:: %bare_comment
-Additional info:: %reporter, %bare_backtrace
+Additional info:: %bare_backtrace
%attach:: dmesg
diff --git a/src/plugins/bugzilla_format_libreport.conf b/src/plugins/bugzilla_format_libreport.conf
index 2ecdf6f..352be98 100644
--- a/src/plugins/bugzilla_format_libreport.conf
+++ b/src/plugins/bugzilla_format_libreport.conf
@@ -50,7 +50,6 @@ Additional info:: \
-analyzer,-count,-duphash,-uuid,-abrt_version,\
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
- %reporter,\
%oneline
Truncated backtrace:: %bare_%short_backtrace
diff --git a/src/plugins/bugzilla_formatdup.conf b/src/plugins/bugzilla_formatdup.conf
index f5e89f7..6f51606 100644
--- a/src/plugins/bugzilla_formatdup.conf
+++ b/src/plugins/bugzilla_formatdup.conf
@@ -49,5 +49,4 @@
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
-var_log_messages,\
- %reporter,\
%oneline
diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c
index 05439cd..cd3cf15 100644
--- a/src/plugins/reporter-bugzilla.c
+++ b/src/plugins/reporter-bugzilla.c
@@ -405,10 +405,6 @@ int append_item(struct strbuf *result, const char *item_name, problem_data_t *pd
if (strcmp(item_name, "%short_backtrace") == 0)
return append_short_backtrace(result, pd, CD_TEXT_ATT_SIZE_BZ, print_item_name);
- /* Compat with previously-existed ad-hockery: %reporter */
- if (strcmp(item_name, "%reporter") == 0)
- return append_text(result, "reporter", PACKAGE"-"VERSION, print_item_name);
-
/* %oneline,%multiline,%text */
bool oneline = (strcmp(item_name+1, "oneline" ) == 0);
bool multiline = (strcmp(item_name+1, "multiline") == 0);

[Jiri Moskovcak]

Ok, I can create it myself, but the patch is missing the ticket number.

--Jirka

Post by Jiri Moskovcak
Have you tried to create a bz with your patch, I'd like to take a look
at the results.
Thanks,
Jirka

Post by Denys Vlasenko
Rationale for %reporter was that created BZ's need information
which version of libreport was creating them.
But the same thing will be needed by other users of our data.
Adding it only in BZ reporter is a wrong place.
We already create "abrt_version" element, I copied the logic
and this change creates similar "libreport_version" element.
BZ reporter will show it along with other one-liners.
---
src/include/internal_libreport.h | 1 +
src/lib/dump_dir.c | 2 ++
src/plugins/bugzilla_format.conf | 1 -
src/plugins/bugzilla_format_kernel.conf | 2 +-
src/plugins/bugzilla_format_libreport.conf | 1 -
src/plugins/bugzilla_formatdup.conf | 1 -
src/plugins/reporter-bugzilla.c | 4 ----
7 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/include/internal_libreport.h
b/src/include/internal_libreport.h
index ae46ad4..6076823 100644
--- a/src/include/internal_libreport.h
+++ b/src/include/internal_libreport.h
@@ -861,6 +861,7 @@ struct dump_dir *open_directory_for_writing(
#define FILENAME_PKG_ARCH "pkg_arch"
#define FILENAME_USERNAME "username"
#define FILENAME_ABRT_VERSION "abrt_version"
+#define FILENAME_LIBREPORT_VERSION "libreport_version"
#define CD_DUMPDIR "Directory"
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
index 4bb23ba..9ff51b4 100644
--- a/src/lib/dump_dir.c
+++ b/src/lib/dump_dir.c
@@ -629,6 +629,8 @@ void dd_create_basic_files(struct dump_dir *dd,
uid_t uid, const char *chroot_di
}
free(time_str);
+ dd_save_text(dd, FILENAME_LIBREPORT_VERSION, PACKAGE"-"VERSION);
+
/* it doesn't make sense to create the uid file if uid == -1 */
if (uid != (uid_t)-1L)
{
diff --git a/src/plugins/bugzilla_format.conf
b/src/plugins/bugzilla_format.conf
index cad5876..f928799 100644
--- a/src/plugins/bugzilla_format.conf
+++ b/src/plugins/bugzilla_format.conf
@@ -48,7 +48,6 @@ Additional info:: \
-analyzer,-count,-duphash,-uuid,-abrt_version,\
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
- %reporter,\
%oneline
Truncated backtrace:: %bare_%short_backtrace
diff --git a/src/plugins/bugzilla_format_kernel.conf
b/src/plugins/bugzilla_format_kernel.conf
index e46af89..c32e424 100644
--- a/src/plugins/bugzilla_format_kernel.conf
+++ b/src/plugins/bugzilla_format_kernel.conf
@@ -40,6 +40,6 @@
Description of problem:: %bare_comment
-Additional info:: %reporter, %bare_backtrace
+Additional info:: %bare_backtrace
%attach:: dmesg
diff --git a/src/plugins/bugzilla_format_libreport.conf
b/src/plugins/bugzilla_format_libreport.conf
index 2ecdf6f..352be98 100644
--- a/src/plugins/bugzilla_format_libreport.conf
+++ b/src/plugins/bugzilla_format_libreport.conf
@@ -50,7 +50,6 @@ Additional info:: \
-analyzer,-count,-duphash,-uuid,-abrt_version,\
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
- %reporter,\
%oneline
Truncated backtrace:: %bare_%short_backtrace
diff --git a/src/plugins/bugzilla_formatdup.conf
b/src/plugins/bugzilla_formatdup.conf
index f5e89f7..6f51606 100644
--- a/src/plugins/bugzilla_formatdup.conf
+++ b/src/plugins/bugzilla_formatdup.conf
@@ -49,5 +49,4 @@
-username,-hostname,-os_release,-os_info,\
-time,-pid,-pwd,-last_occurrence,-ureports_counter,\
-var_log_messages,\
- %reporter,\
%oneline
diff --git a/src/plugins/reporter-bugzilla.c
b/src/plugins/reporter-bugzilla.c
index 05439cd..cd3cf15 100644
--- a/src/plugins/reporter-bugzilla.c
+++ b/src/plugins/reporter-bugzilla.c
@@ -405,10 +405,6 @@ int append_item(struct strbuf *result, const char
*item_name, problem_data_t *pd
if (strcmp(item_name, "%short_backtrace") == 0)
return append_short_backtrace(result, pd,
CD_TEXT_ATT_SIZE_BZ, print_item_name);
- /* Compat with previously-existed ad-hockery: %reporter */
- if (strcmp(item_name, "%reporter") == 0)
- return append_text(result, "reporter", PACKAGE"-"VERSION, print_item_name);
-
/* %oneline,%multiline,%text */
bool oneline = (strcmp(item_name+1, "oneline" ) == 0);
bool multiline = (strcmp(item_name+1, "multiline") == 0);

[Jakub Filak]

If you add a new element like this one (libreport_version) and remove %
reporter keyword, you will see version of libreport used to create the
dump directory but you won't see the version of libreport used to file
the bug tracking ticket. But we need to know even the later mentioned
version because of bugs in formatting and so on. I've added the %
reporter keyword because the previous formatting algorithm provided this
information and I find it really useful.

This earlier commit removes libreport_version from bugzilla tickets:
https://github.com/abrt/libreport/commit/8a456dba7534299be5558df8bbd448dd2f40a170

This later commit adds reporter's version to bugzilla tickets:
https://github.com/abrt/libreport/commit/9d294341f62be3d3e2716eb6111cfe61a21d9c99


Regards,
Jakub

[Denys Vlasenko]

Post by Jakub Filak
If you add a new element like this one (libreport_version) and remove
%reporter keyword, you will see version of libreport used to create the
dump directory but you won't see the version of libreport used to file
the bug tracking ticket. But we need to know even the later mentioned
version because of bugs in formatting and so on.

There are more steps in problem processing,
by this logic we need to know libreport version of every step
separately, since they can be different too.

I think that since the version is usually the same on every step,
it is enough to record it once.

Post by Jakub Filak
I've added the
%reporter keyword because the previous formatting algorithm provided this
information and I find it really useful.

I agree that it's useful.

However, why adding a special case for it when we already have a standard
mechanism for adding information to problems (i.e. we add new elements
to problem directories)?

[Jakub Filak]

CLARIFICATION:

I don't insist on usage of %reporter keyword but I want to see the
reporter version.

Post by Denys Vlasenko

Post by Jakub Filak
If you add a new element like this one (libreport_version) and remove
%reporter keyword, you will see version of libreport used to create the
dump directory but you won't see the version of libreport used to file
the bug tracking ticket. But we need to know even the later mentioned
version because of bugs in formatting and so on.

There are more steps in problem processing,
by this logic we need to know libreport version of every step
separately, since they can be different too.

But there are only two interesting steps -> create and report.
The version from create step is covered by abrt_version.
The version from report step is covered by %reporter keyword.

Post by Denys Vlasenko
I think that since the version is usually the same on every step,
it is enough to record it once.
From the emergency dump directory analysis, I know that users often

reports old dump directories with a newer version of libreport (I'm
sorry, but cannot give you any numbers), therefore I'd say that your
assumption that the version is usually the same on every step is not
valid.

When a user reports a problem, libreport runs all necessary events
(analyze, collect, report) and the events uses the currently installed
libreport tools and not those which created the dump directory.

When I triage ABRT bugs, I can usually deduce the creator version from
abrt_version element but I cannot deduce the reporter version from any
information.

Post by Denys Vlasenko

Post by Jakub Filak
I've added the
%reporter keyword because the previous formatting algorithm provided this
information and I find it really useful.

I agree that it's useful.
However, why adding a special case for it when we already have a standard
mechanism for adding information to problems (i.e. we add new elements
to problem directories)?

I don't find the creator version very useful. As I wrote above, I can
deduce the creator version from abrt_version element. The reporter
version is necessary for fixing bugs in the reporting process.