--- src/include/internal_libreport.h | 4 +++- src/lib/iso_date_string.c | 10 +++++++++- 2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h index d7b3257..616ea0d 100644 --- a/src/include/internal_libreport.h +++ b/src/include/internal_libreport.h @@ -533,7 +533,9 @@ char* get_environ(pid_t pid); * Returns "YYYY-MM-DD-hh:mm:ss" string. */ #define iso_date_string libreport_iso_date_string -char *iso_date_string(time_t *pt); +char *iso_date_string(const time_t *pt); +#define string_iso_date libreport_string_iso_date +time_t string_iso_date(const char *date);
enum { MAKEDESC_SHOW_FILES = (1 << 0), diff --git a/src/lib/iso_date_string.c b/src/lib/iso_date_string.c index e7d01cd..012ded3 100644 --- a/src/lib/iso_date_string.c +++ b/src/lib/iso_date_string.c @@ -19,7 +19,7 @@
#include "internal_libreport.h"
-char *iso_date_string(time_t *pt) +char *iso_date_string(const time_t *pt) { static char buf[sizeof("YYYY-MM-DD-HH:MM:SS") + 4];
@@ -29,3 +29,11 @@ char *iso_date_string(time_t *pt)
return buf; } + +time_t string_iso_date(const char *date) +{ + struct tm tm; + strptime(date, "%Y-%m-%d-%H:%M:%S", &tm); + + return mktime(&tm); +}
--- src/include/internal_libreport.h | 21 ++++++++ src/lib/Makefile.am | 1 + src/lib/report_result.c | 100 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 0 deletions(-) create mode 100644 src/lib/report_result.c
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h index 616ea0d..1959d40 100644 --- a/src/include/internal_libreport.h +++ b/src/include/internal_libreport.h @@ -650,9 +650,30 @@ enum { EVENT_LOG_LOW_WATERMARK = 20 * 1024, };
+enum report_result_type { + REPORT_RESULT_TYPE_URL, + REPORT_RESULT_TYPE_MESSAGE +}; + +struct report_result { + char *event; + char *data; + enum report_result_type type; + time_t timestamp; +}; + #define add_reported_to libreport_add_reported_to void add_reported_to(struct dump_dir *dd, const char *line);
+#define new_report_result libreport_new_report_result +struct report_result *new_report_result(enum report_result_type type, char *data); +#define format_report_result libreport_format_report_result +char *format_report_result(const struct report_result *result); +#define parse_report_result libreport_parse_report_result +struct report_result *parse_report_result(const char *text); +#define free_report_result libreport_free_report_result +void free_report_result(struct report_result *result); + #define log_problem_data libreport_log_problem_data void log_problem_data(problem_data_t *problem_data, const char *pfx);
diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am index 7b870d9..0dac824 100644 --- a/src/lib/Makefile.am +++ b/src/lib/Makefile.am @@ -46,6 +46,7 @@ libreport_la_SOURCES = \ event_config.c \ kernel-tainted.c \ report.c \ + report_result.c \ client.c libreport_la_CPPFLAGS = \ -Wall -Wwrite-strings -Werror \ diff --git a/src/lib/report_result.c b/src/lib/report_result.c new file mode 100644 index 0000000..0d11b1d --- /dev/null +++ b/src/lib/report_result.c @@ -0,0 +1,100 @@ +/* + Copyright (C) 2011 ABRT Team + Copyright (C) 2011 RedHat inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License along + with this program; if not, write to the Free Software Foundation, Inc., + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ +#include "internal_libreport.h" + +struct report_result *new_report_result(enum report_result_type type, char *data) +{ + struct report_result *res; + const char *event; + + if (!(event = getenv("EVENT"))) + event = "unknown"; + + res = xmalloc(sizeof (*res)); + res->event = xstrdup(event); + res->data = data; + res->type = type; + res->timestamp = time(NULL); + + return res; +} + +char *format_report_result(const struct report_result *result) +{ + const char *type_string; + + switch (result->type) + { + case REPORT_RESULT_TYPE_URL: + type_string = "URL"; + break; + case REPORT_RESULT_TYPE_MESSAGE: + type_string = "MSG"; + break; + default: + assert(0); + } + return xasprintf("%s: TIME=%s %s=%s", result->event, + iso_date_string(&result->timestamp), type_string, result->data); +} + +struct report_result *parse_report_result(const char *text) +{ + struct report_result *res; + enum report_result_type type; + time_t ts; + int event_len, event_ts_len; + char event[256], timestamp[256], *data; + + if (sscanf(text, "%s%n %s%n", event, &event_len, timestamp, &event_ts_len) != 2) + return NULL; + + data = skip_whitespace(text + event_ts_len); + if (!strncmp(data, "URL=", 4)) + { + data += 4; + type = REPORT_RESULT_TYPE_URL; + } + else if (!strncmp(data, "MSG=", 4)) + { + data += 4; + type = REPORT_RESULT_TYPE_MESSAGE; + } + else + return NULL; + + if (strncmp(timestamp, "TIME=", 5)) + return NULL; + ts = string_iso_date(timestamp + 5); + + res = xmalloc(sizeof (*res)); + res->event = xstrndup(event, event_len - 1); + res->data = xstrdup(data); + res->type = type; + res->timestamp = ts; + + return res; +} + +void free_report_result(struct report_result *result) +{ + free(result->event); + free(result->data); + free(result); +}
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+#define parse_report_result libreport_parse_report_result +struct report_result *parse_report_result(const char *text);
I object to committing to git of interfaces without users.
I mean: if you need this function for something, then add this function and immediately add the code which uses it.
These can be two separate commits (in fact, it is preferred to have them as separate commits), but they should be committed together, say, during same day.
Otherwise, we end up with the state when we have unused API in the tree.
If the code which uses your new function is not ready, postpone its inclusion into the tree. Maybe you will need to change the API. Maybe you will find out that function is not needed after all.
Today's git has unused parse_report_result(), and string_iso_date() is used only by this unused function, thus it is also dead code.
Another example is cmp_problem_data() function added on July 27 by Nikola. Still as of today, no users in the tree.
On 08/02/2011 02:36 PM, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+#define parse_report_result libreport_parse_report_result +struct report_result *parse_report_result(const char *text);
I object to committing to git of interfaces without users.
- this is actually my fault, I asked Mirek to push it, as mine changes in the gui will use it - but I got overwhelmed by other problems, so I didn't get to it. The idea is to be able to parser report_to and show it in gui in some usable (clickable) way.
I mean: if you need this function for something, then add this function and immediately add the code which uses it.
These can be two separate commits (in fact, it is preferred to have them as separate commits), but they should be committed together, say, during same day.
Otherwise, we end up with the state when we have unused API in the tree.
If the code which uses your new function is not ready, postpone its inclusion into the tree. Maybe you will need to change the API. Maybe you will find out that function is not needed after all.
Today's git has unused parse_report_result(), and string_iso_date() is used only by this unused function, thus it is also dead code.
Another example is cmp_problem_data() function added on July 27 by Nikola. Still as of today, no users in the tree.
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
- char event[256], timestamp[256], *data;
- if (sscanf(text, "%s%n %s%n", event, &event_len, timestamp, &event_ts_len) != 2)
return NULL;
This is buggy wrt buffer overflow.
--- src/lib/report_result.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/lib/report_result.c b/src/lib/report_result.c index 8780dca..f3cad00 100644 --- a/src/lib/report_result.c +++ b/src/lib/report_result.c @@ -73,7 +73,7 @@ struct report_result *parse_report_result(const char *line) int event_len, event_ts_len; char event[256], timestamp[256], *data;
- if (sscanf(line, "%s%n %s%n", event, &event_len, timestamp, &event_ts_len) != 2) + if (sscanf(line, "%255s%n %255s%n", event, &event_len, timestamp, &event_ts_len) != 2) return NULL;
data = skip_whitespace(line + event_ts_len);
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+char *format_report_result(const struct report_result *result) +{
- const char *type_string;
- switch (result->type)
- {
case REPORT_RESULT_TYPE_URL:type_string = "URL";break;case REPORT_RESULT_TYPE_MESSAGE:type_string = "MSG";break;default:assert(0);- }
- return xasprintf("%s: TIME=%s %s=%s", result->event,
iso_date_string(&result->timestamp), type_string, result->data);+}
Before this patch, the prefix indicated the type of report:
RHTSupport: <DATA>
meant "we reported it to RHTSsupport". This is needed to be able to figure out what <DATA> means.
Now, you print event name instead. What will happen if reporting is hooked, for example, to "post-create" event? It will be:
post-create: <DATA>
Now it is impossible to figure out, was it reported to RHTSupport? Bugzilla? Sent by email?
On 08/02/2011 03:42 PM, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+char *format_report_result(const struct report_result *result) +{
- const char *type_string;
- switch (result->type)
- {
case REPORT_RESULT_TYPE_URL:type_string = "URL";break;case REPORT_RESULT_TYPE_MESSAGE:type_string = "MSG";break;default:assert(0);- }
- return xasprintf("%s: TIME=%s %s=%s", result->event,
iso_date_string(&result->timestamp), type_string, result->data);+}
Before this patch, the prefix indicated the type of report:
RHTSupport:<DATA>
meant "we reported it to RHTSsupport". This is needed to be able to figure out what<DATA> means.
Now, you print event name instead. What will happen if reporting is hooked, for example, to "post-create" event? It will be:
post-create:<DATA>
Now it is impossible to figure out, was it reported to RHTSupport? Bugzilla? Sent by email?
actually it adds the event name not the even type:
report_Logger: TIME=2011-07-22-23:32:03 URL=file:///home/jmoskovc/abrt.log
report_Bugzilla: TIME=2011-07-29-10:35:32 URL=https://bugzilla.redhat.com/show_bug.cgi?id=714989
- we can probably remove the report_ but it's easier to look for the matching event, if we have the whole name...
On 08/02/2011 03:56 PM, Jiri Moskovcak wrote:
On 08/02/2011 03:42 PM, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+char *format_report_result(const struct report_result *result) +{
- const char *type_string;
- switch (result->type)
- {
- case REPORT_RESULT_TYPE_URL:
- type_string = "URL";
- break;
- case REPORT_RESULT_TYPE_MESSAGE:
- type_string = "MSG";
- break;
- default:
- assert(0);
- }
- return xasprintf("%s: TIME=%s %s=%s", result->event,
- iso_date_string(&result->timestamp), type_string, result->data);
+}
Before this patch, the prefix indicated the type of report:
RHTSupport:<DATA>
meant "we reported it to RHTSsupport". This is needed to be able to figure out what<DATA> means.
Now, you print event name instead. What will happen if reporting is hooked, for example, to "post-create" event? It will be:
post-create:<DATA>
Now it is impossible to figure out, was it reported to RHTSupport? Bugzilla? Sent by email?
actually it adds the event name not the even type:
report_Logger: TIME=2011-07-22-23:32:03 URL=file:///home/jmoskovc/abrt.log
report_Bugzilla: TIME=2011-07-29-10:35:32 URL=https://bugzilla.redhat.com/show_bug.cgi?id=714989
- we can probably remove the report_ but it's easier to look for the
matching event, if we have the whole name...
- oh, now I get it... I wanted the event name because 2 events can share the same reporter i.e
EVENT=report_BugzillaFedora reporter-bugzilla EVENT=report_BugzillaMandriva reporter-bugzilla -c conffile.conf
the in reported_to would be:
bugzilla: URL=bugzilla.redhat.com bugzilla: URL=bugzilla.mandriva.com
and I wanted to show
BugzillaMandriva: url BugzillaFedora: url
but without the event name I only can show:
bugzilla: url bugzilla: different_url
On 08/02/2011 04:01 PM, Jiri Moskovcak wrote:
On 08/02/2011 03:56 PM, Jiri Moskovcak wrote:
On 08/02/2011 03:42 PM, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+char *format_report_result(const struct report_result *result) +{
- const char *type_string;
- switch (result->type)
- {
- case REPORT_RESULT_TYPE_URL:
- type_string = "URL";
- break;
- case REPORT_RESULT_TYPE_MESSAGE:
- type_string = "MSG";
- break;
- default:
- assert(0);
- }
- return xasprintf("%s: TIME=%s %s=%s", result->event,
- iso_date_string(&result->timestamp), type_string, result->data);
+}
Before this patch, the prefix indicated the type of report:
RHTSupport:<DATA>
meant "we reported it to RHTSsupport". This is needed to be able to figure out what<DATA> means.
Now, you print event name instead. What will happen if reporting is hooked, for example, to "post-create" event? It will be:
post-create:<DATA>
Now it is impossible to figure out, was it reported to RHTSupport? Bugzilla? Sent by email?
actually it adds the event name not the even type:
report_Logger: TIME=2011-07-22-23:32:03 URL=file:///home/jmoskovc/abrt.log
report_Bugzilla: TIME=2011-07-29-10:35:32 URL=https://bugzilla.redhat.com/show_bug.cgi?id=714989
- we can probably remove the report_ but it's easier to look for the
matching event, if we have the whole name...
- oh, now I get it... I wanted the event name because 2 events can share
the same reporter i.e
EVENT=report_BugzillaFedora reporter-bugzilla EVENT=report_BugzillaMandriva reporter-bugzilla -c conffile.conf
the in reported_to would be:
bugzilla: URL=bugzilla.redhat.com bugzilla: URL=bugzilla.mandriva.com
and I wanted to show
BugzillaMandriva: url BugzillaFedora: url
but without the event name I only can show:
bugzilla: url bugzilla: different_url
so generally I wanted to be able to find a xml and screen_name for event responsible for the line in reported_to
On Tue, 2011-08-02 at 16:04 +0200, Jiri Moskovcak wrote:
On 08/02/2011 04:01 PM, Jiri Moskovcak wrote:
On 08/02/2011 03:56 PM, Jiri Moskovcak wrote:
On 08/02/2011 03:42 PM, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
+char *format_report_result(const struct report_result *result) +{
- const char *type_string;
- switch (result->type)
- {
- case REPORT_RESULT_TYPE_URL:
- type_string = "URL";
- break;
- case REPORT_RESULT_TYPE_MESSAGE:
- type_string = "MSG";
- break;
- default:
- assert(0);
- }
- return xasprintf("%s: TIME=%s %s=%s", result->event,
- iso_date_string(&result->timestamp), type_string, result->data);
+}
Before this patch, the prefix indicated the type of report:
RHTSupport:<DATA>
meant "we reported it to RHTSsupport". This is needed to be able to figure out what<DATA> means.
Now, you print event name instead. What will happen if reporting is hooked, for example, to "post-create" event? It will be:
post-create:<DATA>
Now it is impossible to figure out, was it reported to RHTSupport? Bugzilla? Sent by email?
actually it adds the event name not the even type:
report_Logger: TIME=2011-07-22-23:32:03 URL=file:///home/jmoskovc/abrt.log
report_Bugzilla: TIME=2011-07-29-10:35:32 URL=https://bugzilla.redhat.com/show_bug.cgi?id=714989
- we can probably remove the report_ but it's easier to look for the
matching event, if we have the whole name...
- oh, now I get it... I wanted the event name because 2 events can share
the same reporter i.e
EVENT=report_BugzillaFedora reporter-bugzilla EVENT=report_BugzillaMandriva reporter-bugzilla -c conffile.conf
the in reported_to would be:
bugzilla: URL=bugzilla.redhat.com bugzilla: URL=bugzilla.mandriva.com
and I wanted to show
BugzillaMandriva: url BugzillaFedora: url
but without the event name I only can show:
bugzilla: url bugzilla: different_url
so generally I wanted to be able to find a xml and screen_name for event responsible for the line in reported_to
I don't see for what purpose you want to know event name, but anyway. I can be achieved rather easily like this:
bugzilla: EVENT=event URL=url ...
--- src/plugins/reporter-bugzilla.c | 7 ++++++- src/plugins/reporter-kerneloops.c | 7 ++++++- src/plugins/reporter-mailx.c | 7 ++++++- src/plugins/reporter-print.c | 7 ++++++- src/plugins/reporter-rhtsupport.c | 7 ++++++- 5 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c index 99b03e7..5b58a29 100644 --- a/src/plugins/reporter-bugzilla.c +++ b/src/plugins/reporter-bugzilla.c @@ -207,7 +207,12 @@ static void report_to_bugzilla(const char *dump_dir_name, map_string_h *settings struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - char *msg = xasprintf("Bugzilla: URL=%s/show_bug.cgi?id=%u", bugzilla_url, bz->bi_id); + struct report_result *res; + char *msg = xasprintf("%s/show_bug.cgi?id=%u", bugzilla_url, bz->bi_id); + + res = new_report_result(REPORT_RESULT_TYPE_URL, msg); + msg = format_report_result(res); + free_report_result(res); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-kerneloops.c b/src/plugins/reporter-kerneloops.c index 61fac4c..5db2c4b 100644 --- a/src/plugins/reporter-kerneloops.c +++ b/src/plugins/reporter-kerneloops.c @@ -111,7 +111,12 @@ static void report_to_kerneloops( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - char *msg = xasprintf("kerneloops: URL=%s", submitURL); + struct report_result *res; + char *msg = xasprintf("%s", submitURL); + + res = new_report_result(REPORT_RESULT_TYPE_URL, msg); + msg = format_report_result(res); + free_report_result(res); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-mailx.c b/src/plugins/reporter-mailx.c index 99ac586..b89957d 100644 --- a/src/plugins/reporter-mailx.c +++ b/src/plugins/reporter-mailx.c @@ -114,7 +114,12 @@ static void create_and_send_email( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - char *msg = xasprintf("email: %s", email_to); + struct report_result *res; + char *msg = xasprintf("mailto:%s", email_to); + + res = new_report_result(REPORT_RESULT_TYPE_URL, msg); + msg = format_report_result(res); + free_report_result(res); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-print.c b/src/plugins/reporter-print.c index c688b32..241a22a 100644 --- a/src/plugins/reporter-print.c +++ b/src/plugins/reporter-print.c @@ -81,7 +81,12 @@ int main(int argc, char **argv) struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - char *msg = xasprintf("file: %s", output_file); + struct report_result *res; + char *msg = xasprintf("file://%s", output_file);; + + res = new_report_result(REPORT_RESULT_TYPE_URL, msg); + msg = format_report_result(res); + free_report_result(res); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-rhtsupport.c b/src/plugins/reporter-rhtsupport.c index 4f6d2b6..eade3ee 100644 --- a/src/plugins/reporter-rhtsupport.c +++ b/src/plugins/reporter-rhtsupport.c @@ -243,7 +243,12 @@ static void report_to_rhtsupport( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - char *msg = xasprintf("RHTSupport: %s", result); + struct report_result *res; + char *msg = xasprintf("%s", result); + + res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg); + msg = format_report_result(res); + free_report_result(res); add_reported_to(dd, msg); free(msg); dd_close(dd);
--- src/include/internal_libreport.h | 4 ++-- src/lib/iso_date_string.c | 2 +- src/lib/report_result.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h index 1959d40..ccfd597 100644 --- a/src/include/internal_libreport.h +++ b/src/include/internal_libreport.h @@ -534,8 +534,8 @@ char* get_environ(pid_t pid); */ #define iso_date_string libreport_iso_date_string char *iso_date_string(const time_t *pt); -#define string_iso_date libreport_string_iso_date -time_t string_iso_date(const char *date); +#define parse_iso_date_string libreport_parse_iso_date_string +time_t parse_iso_date_string(const char *date);
enum { MAKEDESC_SHOW_FILES = (1 << 0), diff --git a/src/lib/iso_date_string.c b/src/lib/iso_date_string.c index 012ded3..3cf30c7 100644 --- a/src/lib/iso_date_string.c +++ b/src/lib/iso_date_string.c @@ -30,7 +30,7 @@ char *iso_date_string(const time_t *pt) return buf; }
-time_t string_iso_date(const char *date) +time_t parse_iso_date_string(const char *date) { struct tm tm; strptime(date, "%Y-%m-%d-%H:%M:%S", &tm); diff --git a/src/lib/report_result.c b/src/lib/report_result.c index 0d11b1d..ddfc24b 100644 --- a/src/lib/report_result.c +++ b/src/lib/report_result.c @@ -81,7 +81,7 @@ struct report_result *parse_report_result(const char *text)
if (strncmp(timestamp, "TIME=", 5)) return NULL; - ts = string_iso_date(timestamp + 5); + ts = parse_iso_date_string(timestamp + 5);
res = xmalloc(sizeof (*res)); res->event = xstrndup(event, event_len - 1);
--- src/include/internal_libreport.h | 4 +++- src/lib/report_result.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h index ccfd597..8758166 100644 --- a/src/include/internal_libreport.h +++ b/src/include/internal_libreport.h @@ -670,9 +670,11 @@ struct report_result *new_report_result(enum report_result_type type, char *data #define format_report_result libreport_format_report_result char *format_report_result(const struct report_result *result); #define parse_report_result libreport_parse_report_result -struct report_result *parse_report_result(const char *text); +struct report_result *parse_report_result(const char *line); #define free_report_result libreport_free_report_result void free_report_result(struct report_result *result); +#define parse_report_results libreport_parse_report_results +GList *parse_report_results(const char *text);
#define log_problem_data libreport_log_problem_data void log_problem_data(problem_data_t *problem_data, const char *pfx); diff --git a/src/lib/report_result.c b/src/lib/report_result.c index ddfc24b..d379b2f 100644 --- a/src/lib/report_result.c +++ b/src/lib/report_result.c @@ -54,7 +54,7 @@ char *format_report_result(const struct report_result *result) iso_date_string(&result->timestamp), type_string, result->data); }
-struct report_result *parse_report_result(const char *text) +struct report_result *parse_report_result(const char *line) { struct report_result *res; enum report_result_type type; @@ -62,10 +62,10 @@ struct report_result *parse_report_result(const char *text) int event_len, event_ts_len; char event[256], timestamp[256], *data;
- if (sscanf(text, "%s%n %s%n", event, &event_len, timestamp, &event_ts_len) != 2) + if (sscanf(line, "%s%n %s%n", event, &event_len, timestamp, &event_ts_len) != 2) return NULL;
- data = skip_whitespace(text + event_ts_len); + data = skip_whitespace(line + event_ts_len); if (!strncmp(data, "URL=", 4)) { data += 4; @@ -98,3 +98,30 @@ void free_report_result(struct report_result *result) free(result->data); free(result); } + +GList *parse_report_results(const char *text) +{ + const char *next_line; + char *line; + struct report_result *res; + GList *res_list; + + res_list = NULL; + + while (*text) + { + next_line = strchrnul(text, '\n'); + line = xstrndup(text, next_line - text); + res = parse_report_result(line); + free(line); + + if (res) + res_list = g_list_append(res_list, res); + + text = next_line; + if (*text) + text++; + } + + return res_list; +}
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
src/plugins/reporter-bugzilla.c | 7 ++++++- src/plugins/reporter-kerneloops.c | 7 ++++++- src/plugins/reporter-mailx.c | 7 ++++++- src/plugins/reporter-print.c | 7 ++++++- src/plugins/reporter-rhtsupport.c | 7 ++++++- 5 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c index 99b03e7..5b58a29 100644 --- a/src/plugins/reporter-bugzilla.c +++ b/src/plugins/reporter-bugzilla.c @@ -207,7 +207,12 @@ static void report_to_bugzilla(const char *dump_dir_name, map_string_h *settings struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) {
char *msg = xasprintf("Bugzilla: URL=%s/show_bug.cgi?id=%u", bugzilla_url, bz->bi_id);
struct report_result *res;char *msg = xasprintf("%s/show_bug.cgi?id=%u", bugzilla_url, bz->bi_id);res = new_report_result(REPORT_RESULT_TYPE_URL, msg);msg = format_report_result(res);free_report_result(res);
This looks like a step back to me, not a progress. Instead of one line of code, now you need five.
Intermediate "struct report_result" object is not really needed.
How about a custom result formatting function which simply returns char*?
if (dd) {
char *msg = xasprintf("kerneloops: URL=%s", submitURL);
struct report_result *res;char *msg = xasprintf("%s", submitURL);res = new_report_result(REPORT_RESULT_TYPE_URL, msg);msg = format_report_result(res);free_report_result(res);
Same.
if (dd) {
char *msg = xasprintf("email: %s", email_to);
struct report_result *res;char *msg = xasprintf("mailto:%s", email_to);res = new_report_result(REPORT_RESULT_TYPE_URL, msg);msg = format_report_result(res);free_report_result(res);
Same.
if (dd) {
char *msg = xasprintf("file: %s", output_file);
struct report_result *res;char *msg = xasprintf("file://%s", output_file);;res = new_report_result(REPORT_RESULT_TYPE_URL, msg);msg = format_report_result(res);free_report_result(res); add_reported_to(dd, msg);
Same.
free(msg); dd_close(dd);diff --git a/src/plugins/reporter-rhtsupport.c b/src/plugins/reporter-rhtsupport.c index 4f6d2b6..eade3ee 100644 --- a/src/plugins/reporter-rhtsupport.c +++ b/src/plugins/reporter-rhtsupport.c @@ -243,7 +243,12 @@ static void report_to_rhtsupport( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) {
char *msg = xasprintf("RHTSupport: %s", result);
struct report_result *res;char *msg = xasprintf("%s", result);res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg);msg = format_report_result(res);free_report_result(res);
Same.
--- src/include/internal_libreport.h | 2 ++ src/lib/report_result.c | 11 +++++++++++ src/plugins/reporter-bugzilla.c | 8 ++------ src/plugins/reporter-kerneloops.c | 8 ++------ src/plugins/reporter-mailx.c | 8 ++------ src/plugins/reporter-print.c | 8 ++------ src/plugins/reporter-rhtsupport.c | 8 ++------ 7 files changed, 23 insertions(+), 30 deletions(-)
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h index 8758166..8ed749c 100644 --- a/src/include/internal_libreport.h +++ b/src/include/internal_libreport.h @@ -669,6 +669,8 @@ void add_reported_to(struct dump_dir *dd, const char *line); struct report_result *new_report_result(enum report_result_type type, char *data); #define format_report_result libreport_format_report_result char *format_report_result(const struct report_result *result); +#define format_new_report_result libreport_format_new_report_result +char *format_new_report_result(enum report_result_type type, char *data); #define parse_report_result libreport_parse_report_result struct report_result *parse_report_result(const char *line); #define free_report_result libreport_free_report_result diff --git a/src/lib/report_result.c b/src/lib/report_result.c index d379b2f..8780dca 100644 --- a/src/lib/report_result.c +++ b/src/lib/report_result.c @@ -54,6 +54,17 @@ char *format_report_result(const struct report_result *result) iso_date_string(&result->timestamp), type_string, result->data); }
+char *format_new_report_result(enum report_result_type type, char *data) +{ + struct report_result *result; + char *line; + + result = new_report_result(type, data); + line = format_report_result(result); + free_report_result(result); + return line; +} + struct report_result *parse_report_result(const char *line) { struct report_result *res; diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c index 5b58a29..a17f7d3 100644 --- a/src/plugins/reporter-bugzilla.c +++ b/src/plugins/reporter-bugzilla.c @@ -207,12 +207,8 @@ static void report_to_bugzilla(const char *dump_dir_name, map_string_h *settings struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - struct report_result *res; - char *msg = xasprintf("%s/show_bug.cgi?id=%u", bugzilla_url, bz->bi_id); - - res = new_report_result(REPORT_RESULT_TYPE_URL, msg); - msg = format_report_result(res); - free_report_result(res); + char *msg = format_new_report_result(REPORT_RESULT_TYPE_URL, + xasprintf("%s/show_bug.cgi?id=%u", bugzilla_url, bz->bi_id)); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-kerneloops.c b/src/plugins/reporter-kerneloops.c index 5db2c4b..d9c8fbc 100644 --- a/src/plugins/reporter-kerneloops.c +++ b/src/plugins/reporter-kerneloops.c @@ -111,12 +111,8 @@ static void report_to_kerneloops( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - struct report_result *res; - char *msg = xasprintf("%s", submitURL); - - res = new_report_result(REPORT_RESULT_TYPE_URL, msg); - msg = format_report_result(res); - free_report_result(res); + char *msg = format_new_report_result(REPORT_RESULT_TYPE_URL, + xasprintf("%s", submitURL)); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-mailx.c b/src/plugins/reporter-mailx.c index b89957d..ca83c52 100644 --- a/src/plugins/reporter-mailx.c +++ b/src/plugins/reporter-mailx.c @@ -114,12 +114,8 @@ static void create_and_send_email( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - struct report_result *res; - char *msg = xasprintf("mailto:%s", email_to); - - res = new_report_result(REPORT_RESULT_TYPE_URL, msg); - msg = format_report_result(res); - free_report_result(res); + char *msg = format_new_report_result(REPORT_RESULT_TYPE_URL, + xasprintf("mailto:%s", email_to)); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-print.c b/src/plugins/reporter-print.c index 241a22a..8a4d8b8 100644 --- a/src/plugins/reporter-print.c +++ b/src/plugins/reporter-print.c @@ -81,12 +81,8 @@ int main(int argc, char **argv) struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - struct report_result *res; - char *msg = xasprintf("file://%s", output_file);; - - res = new_report_result(REPORT_RESULT_TYPE_URL, msg); - msg = format_report_result(res); - free_report_result(res); + char *msg = format_new_report_result(REPORT_RESULT_TYPE_URL, + xasprintf("file://%s", output_file)); add_reported_to(dd, msg); free(msg); dd_close(dd); diff --git a/src/plugins/reporter-rhtsupport.c b/src/plugins/reporter-rhtsupport.c index eade3ee..63b39d1 100644 --- a/src/plugins/reporter-rhtsupport.c +++ b/src/plugins/reporter-rhtsupport.c @@ -243,12 +243,8 @@ static void report_to_rhtsupport( struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0); if (dd) { - struct report_result *res; - char *msg = xasprintf("%s", result); - - res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg); - msg = format_report_result(res); - free_report_result(res); + char *msg = format_new_report_result(REPORT_RESULT_TYPE_MESSAGE, + xasprintf("%s", result)); add_reported_to(dd, msg); free(msg); dd_close(dd);
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
char *msg = xasprintf("%s", result);res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg);msg = format_report_result(res); <--- HERE
Memory leak: xasprintf result is lost.
On Tue, Aug 02, 2011 at 03:36:20PM +0200, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
char *msg = xasprintf("%s", result);res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg);msg = format_report_result(res); <--- HEREMemory leak: xasprintf result is lost.
It's freed in the free_report_result call.
On Tue, 2011-08-02 at 15:38 +0200, Miroslav Lichvar wrote:
On Tue, Aug 02, 2011 at 03:36:20PM +0200, Denys Vlasenko wrote:
On Fri, 2011-07-22 at 13:37 +0200, Miroslav Lichvar wrote:
char *msg = xasprintf("%s", result);res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg);msg = format_report_result(res); <--- HEREMemory leak: xasprintf result is lost.
It's freed in the free_report_result call.
I don't see it. Let me walk it through:
char *msg = xasprintf("%s", result); ^^^^^^^^^^^^^ we allocated new string, msg res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg); ^^^^^^^^^^^^^ we use it to build res msg = format_report_result(res); ^^^^^^^^^^^^^ we create final message string from res and assign it to msg. Here we lost old msg pointer during assignment. It's leaked.
On Tue, Aug 02, 2011 at 04:58:19PM +0200, Denys Vlasenko wrote:
I don't see it. Let me walk it through:
char *msg = xasprintf("%s", result);^^^^^^^^^^^^^ we allocated new string, msg res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg); ^^^^^^^^^^^^^ we use it to build res
new_report_result accepts char *, msg is now owned by the struct
msg = format_report_result(res);^^^^^^^^^^^^^ we create final message string from res and assign it to msg. Here we lost old msg pointer during assignment. It's leaked.
free_report_result(res);
The original msg is now freed.
If you think new_report_result should accept const char * instead, we can add the extra strdup+free calls.
On Tue, 2011-08-02 at 17:26 +0200, Miroslav Lichvar wrote:
On Tue, Aug 02, 2011 at 04:58:19PM +0200, Denys Vlasenko wrote:
I don't see it. Let me walk it through:
char *msg = xasprintf("%s", result);^^^^^^^^^^^^^ we allocated new string, msg res = new_report_result(REPORT_RESULT_TYPE_MESSAGE, msg); ^^^^^^^^^^^^^ we use it to build res
new_report_result accepts char *, msg is now owned by the struct
Thanks, now I see.
msg = format_report_result(res);^^^^^^^^^^^^^ we create final message string from res and assign it to msg. Here we lost old msg pointer during assignment. It's leaked.
free_report_result(res);
The original msg is now freed.
If you think new_report_result should accept const char * instead, we can add the extra strdup+free calls.
I don't know what is better. Taking ownership is more efficient in some cases; but it's more confusing.
crash-catcher@lists.fedorahosted.org
-
Denys Vlasenko -
Jiri Moskovcak -
Miroslav Lichvar