Hi,
I propose the following attached patches:
Patch 1 removes the wrappers. I don't see why we need them.
Also removes CD_FLAG_FILE, we have CD_FLAG_BIN which is the same.
Patches 2 and 3 simplify save_dump_dir_from_problem_data() code
and the method it uses to pass back the name of the created directory.
Also, move its decl into problem_data.h - I feel it belongs here.
These are pretty mechanical changes, which open up the way for
the next step:
We need to merge create_dump_dir_from_problem_data()
and save_dump_dir_from_problem_data() into one function,
and also reuse this new function instead of open-coded
problem_data -> dump_dir code in abrt-server.
(IOW: we have triple code duplication right now, need to consolidate).
--
vda
diff -x '*.po' -d -urpN abrt.6/src/dbus/abrt-dbus.c abrt.7/src/dbus/abrt-dbus.c
--- abrt.6/src/dbus/abrt-dbus.c 2012-07-10 19:15:17.000000000 +0200
+++ abrt.7/src/dbus/abrt-dbus.c 2012-07-11 10:53:32.120124364 +0200
@@ -362,25 +362,30 @@ static bool allowed_problem_dir(const ch
return true;
}
+/* Saves the problem data
+ * creates the problem_dir in the configured problems directory.
+ */
+static int problem_data_save(problem_data_t *pd, char **problem_id)
+{
+ load_abrt_conf();
+ LibreportError res = save_dump_dir_from_problem_data(pd, problem_id, g_settings_dump_location);
+ VERB2 log("problem id: '%s'", *problem_id);
+
+ return res;
+}
+
static bool handle_new_problem(GVariant *problem_info, char **problem_id, char **error)
{
bool result = true;
- problem_data_t *pd = problem_data_new();
+ problem_data_t *pd = new_problem_data();
GVariantIter *iter;
g_variant_get(problem_info, "a{ss}", &iter);
gchar *key, *value;
while (g_variant_iter_loop(iter, "{ss}", &key, &value))
{
- if (problem_data_add_item(pd, key, value) != 0)
- {
- if (error)
- *error = xasprintf("Cannot save item '%s'", key);
-
- result = false;
- goto handle_new_problem_cleanup;
- }
+ add_to_problem_data_ext(pd, key, value, CD_FLAG_TXT);
}
if (problem_data_save(pd, problem_id) != 0)
@@ -391,8 +396,7 @@ static bool handle_new_problem(GVariant
result = false;
}
-handle_new_problem_cleanup:
- problem_data_destroy(pd);
+ free_problem_data(pd);
return result;
}
diff -x '*.po' -d -urpN abrt.6/src/include/hooklib.h abrt.7/src/include/hooklib.h
--- abrt.6/src/include/hooklib.h 2012-07-10 19:15:17.000000000 +0200
+++ abrt.7/src/include/hooklib.h 1970-01-01 01:00:00.000000000 +0100
@@ -1,23 +0,0 @@
-/*
- Copyright (C) 2009 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.
-*/
-
-problem_data_t * problem_data_new();
-int problem_data_add_item(problem_data_t *pd, char *key, char *value);
-int problem_data_add_file(problem_data_t *pd, const char* path);
-int problem_data_save(problem_data_t *pd, char **problem_id);
-void problem_data_destroy(problem_data_t *pd);
diff -x '*.po' -d -urpN abrt.6/src/lib/hooklib.c abrt.7/src/lib/hooklib.c
--- abrt.6/src/lib/hooklib.c 2012-07-10 19:15:17.000000000 +0200
+++ abrt.7/src/lib/hooklib.c 2012-07-11 10:54:48.850894422 +0200
@@ -342,51 +342,3 @@ char *get_backtrace(const char *dump_dir
free(args[7]);
return bt;
}
-
-static const char *get_filename(const char *path)
-{
- const char *filename = NULL;
-
- filename = strrchr(path, '/');
- if (filename)
- /* +1 => strip the '/' */
- return filename+1;
-
- return path;
-}
-
-problem_data_t *problem_data_new()
-{
- return new_problem_data();
-}
-
-int problem_data_add_item(problem_data_t *pd, char * key, char * value)
-{
- add_to_problem_data_ext(pd, key, value, CD_FLAG_TXT);
- return 0;
-}
-
-int problem_data_add_file(problem_data_t *pd, const char* path)
-{
- add_to_problem_data_ext(pd, get_filename(path), path, CD_FLAG_FILE);
- return 0;
-}
-
-/** Saves the problem data
- * creates the problem_dir in the configured problems directory
- * destroyes
-*/
-int problem_data_save(problem_data_t *pd, char **problem_id)
-{
- load_abrt_conf();
- LibreportError res = save_dump_dir_from_problem_data(pd, problem_id, g_settings_dump_location);
- VERB2 log("problem id: '%s'", *problem_id);
-
- return res;
-}
-
-void problem_data_destroy(problem_data_t *pd)
-{
- free_problem_data(pd);
- pd = NULL;
-}
diff -x '*.po' -d -urpN abrt.7/src/dbus/abrt-dbus.c abrt.8/src/dbus/abrt-dbus.c
--- abrt.7/src/dbus/abrt-dbus.c 2012-07-11 10:53:32.120124364 +0200
+++ abrt.8/src/dbus/abrt-dbus.c 2012-07-11 11:10:59.580580625 +0200
@@ -365,19 +365,16 @@ static bool allowed_problem_dir(const ch
/* Saves the problem data
* creates the problem_dir in the configured problems directory.
*/
-static int problem_data_save(problem_data_t *pd, char **problem_id)
+static char* problem_data_save(problem_data_t *pd)
{
load_abrt_conf();
- LibreportError res = save_dump_dir_from_problem_data(pd, problem_id, g_settings_dump_location);
- VERB2 log("problem id: '%s'", *problem_id);
-
- return res;
+ char *problem_id = save_dump_dir_from_problem_data(pd, g_settings_dump_location);
+ VERB2 log("problem id: '%s'", problem_id);
+ return problem_id;
}
-static bool handle_new_problem(GVariant *problem_info, char **problem_id, char **error)
+static char* handle_new_problem(GVariant *problem_info, char **error)
{
- bool result = true;
-
problem_data_t *pd = new_problem_data();
GVariantIter *iter;
@@ -388,16 +385,15 @@ static bool handle_new_problem(GVariant
add_to_problem_data_ext(pd, key, value, CD_FLAG_TXT);
}
- if (problem_data_save(pd, problem_id) != 0)
+ char *problem_id = problem_data_save(pd);
+ if (!problem_id)
{
if (error)
*error = xasprintf("Cannot create a new problem");
-
- result = false;
}
free_problem_data(pd);
- return result;
+ return problem_id;
}
static void return_InvalidProblemDir_error(GDBusMethodInvocation *invocation, const char *dir_name)
@@ -433,9 +429,9 @@ static void handle_method_call(GDBusConn
if (g_strcmp0(method_name, "NewProblem") == 0)
{
- char *problem_id = NULL;
char *error = NULL;
- if (!handle_new_problem(g_variant_get_child_value(parameters, 0), &problem_id, &error))
+ char *problem_id = handle_new_problem(g_variant_get_child_value(parameters, 0), &error);
+ if (!problem_id)
{
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.Failure",
diff -x '*.po' -d -urpN libreport.0/src/include/problem_data.h libreport.1/src/include/problem_data.h
--- libreport.0/src/include/problem_data.h 2012-07-10 18:07:42.000000000 +0200
+++ libreport.1/src/include/problem_data.h 2012-07-11 11:11:21.867504301 +0200
@@ -35,7 +35,6 @@ enum {
/* Show this element in "short" info (report-cli -l) */
CD_FLAG_LIST = (1 << 4),
CD_FLAG_UNIXTIME = (1 << 5),
- CD_FLAG_FILE = (1 << 6),
};
struct problem_item {
@@ -62,7 +61,6 @@ void add_basics_to_problem_data(problem_
static inline void free_problem_data(problem_data_t *problem_data)
{
- //TODO: leaks problem item;
if (problem_data)
g_hash_table_destroy(problem_data);
}
@@ -94,6 +92,8 @@ problem_data_t *create_problem_data_for_
struct dump_dir *create_dump_dir_from_problem_data(problem_data_t *problem_data, const char *base_dir_name);
+char* save_dump_dir_from_problem_data(problem_data_t *problem_data, const char *base_dir_name);
+
#ifdef __cplusplus
}
#endif
diff -x '*.po' -d -urpN libreport.0/src/include/report.h libreport.1/src/include/report.h
--- libreport.0/src/include/report.h 2012-07-10 18:07:42.000000000 +0200
+++ libreport.1/src/include/report.h 2012-07-11 10:56:42.210524599 +0200
@@ -37,12 +37,6 @@ enum {
LIBREPORT_RUN_NEWT = (1 << 8), /* run 'report-newt' */
};
-typedef enum {
- LR_OK = 0,
- LR_MISSING_ITEM = (1 << 1),
- LR_ERROR = (1 << 2),
-} LibreportError;
-
int report_problem_in_dir(const char *dirname, int flags);
/* Reports a problem stored in problem_data_t.
@@ -53,8 +47,6 @@ int report_problem_in_memory(problem_dat
/* Simple wrapper for trivial uses */
int report_problem(problem_data_t *pd);
-LibreportError save_dump_dir_from_problem_data(problem_data_t *problem_data, char **problem_id, const char *base_dir_name);
-
#ifdef __cplusplus
}
#endif
diff -x '*.po' -d -urpN libreport.0/src/lib/create_dump_dir.c libreport.1/src/lib/create_dump_dir.c
--- libreport.0/src/lib/create_dump_dir.c 2012-07-10 18:07:42.000000000 +0200
+++ libreport.1/src/lib/create_dump_dir.c 2012-07-11 11:18:59.228936241 +0200
@@ -91,39 +91,35 @@ struct dump_dir *create_dump_dir_from_pr
return dd;
}
-LibreportError save_dump_dir_from_problem_data(problem_data_t *problem_data, char **problem_id, const char *base_dir_name)
+char* save_dump_dir_from_problem_data(problem_data_t *problem_data, const char *base_dir_name)
{
char *type = get_problem_item_content_or_NULL(problem_data, FILENAME_TYPE);
- if(!type)
+ if (!type)
{
error_msg(_("Missing required item: '%s'"), FILENAME_TYPE);
- return LR_MISSING_ITEM;
+ return NULL;
}
char *time_s = get_problem_item_content_or_NULL(problem_data, FILENAME_TIME);
- if(!time_s)
+ if (!time_s)
{
- /* 64 is a randomly picked constant which should be
- * enough to hold the time in seconds for a long time..
- */
+ /* time is a required field, so if it's not provided add a default one */
char buf[sizeof(long unsigned)*3];
time_t t = time(NULL);
- /* time is a required field, so if it's not provided add a default one */
- snprintf(buf, sizeof(buf), "%lu", (long unsigned)t);
+ sprintf(buf, "%lu", (long unsigned)t);
add_to_problem_data_ext(problem_data, FILENAME_TIME, buf, CD_FLAG_TXT);
}
- *problem_id = xasprintf("%s-%s-%lu"NEW_PD_SUFFIX, type, iso_date_string(NULL), (long)getpid());
+ char *problem_id = xasprintf("%s-%s-%lu"NEW_PD_SUFFIX, type, iso_date_string(NULL), (long)getpid());
- VERB2 log("Saving to %s/%s", base_dir_name, *problem_id);
- struct dump_dir *dd = try_dd_create(base_dir_name, *problem_id);
+ VERB2 log("Saving to %s/%s", base_dir_name, problem_id);
+ struct dump_dir *dd = try_dd_create(base_dir_name, problem_id);
if (!dd)
{
perror_msg("Can't create problem directory");
- free(*problem_id);
- *problem_id = NULL;
- return LR_ERROR;
+ free(problem_id);
+ return NULL;
}
GHashTableIter iter;
@@ -132,17 +128,15 @@ LibreportError save_dump_dir_from_proble
g_hash_table_iter_init(&iter, problem_data);
while (g_hash_table_iter_next(&iter, (void**)&name, (void**)&value))
{
- if (value->flags & CD_FLAG_FILE)
+ if (value->flags & CD_FLAG_BIN)
{
- char dest[PATH_MAX+1];
- snprintf(dest, sizeof(dest), "%s/%s", dd->dd_dirname, name);
+ char *dest = concat_path_file(dd->dd_dirname, name);
VERB2 log("copying '%s' to '%s'", value->content, dest);
off_t copied = copy_file(value->content, dest, 0644);
if (copied < 0)
error_msg("Can't copy %s to %s", value->content, dest);
-
+ free(dest);
VERB2 log("copied %li bytes", (unsigned long)copied);
-
continue;
}
@@ -153,34 +147,21 @@ LibreportError save_dump_dir_from_proble
continue;
}
-//FIXME: what to do with CD_FLAG_BINs??
- if (value->flags & CD_FLAG_BIN)
- continue;
-
dd_save_text(dd, name, value->content);
}
+ problem_id[strlen(problem_id) - strlen(NEW_PD_SUFFIX)] = '\0';
-
- char *suffix = strstr(*problem_id, NEW_PD_SUFFIX);
- if (suffix)
- *suffix = '\0';
-
- char* new_path = concat_path_file(base_dir_name, *problem_id);
-
+ char *new_path = concat_path_file(base_dir_name, problem_id);
VERB2 log("Renaming from'%s' to '%s'", dd->dd_dirname, new_path);
if (dd_rename(dd, new_path) != 0)
{
- free(new_path);
- dd_close(dd);
-
- free(*problem_id);
- *problem_id = NULL;
- return LR_ERROR;
+ free(problem_id);
+ problem_id = NULL;
}
-
free(new_path);
+
dd_close(dd);
- return LR_OK;
+ return problem_id;
}
