On Tuesday 26 of June 2012 12:34:46 Jiri Moskovcak wrote:
> - should be used in call from hook api,
> maybe it can even replace the former
> create_dump_dir_from_problem_data
> ---
> po/POTFILES.in | 1 +
> src/include/problem_data.h | 2 +
> src/include/report.h | 8 ++++
> src/lib/create_dump_dir.c | 94
> ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 105
> insertions(+)
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 3203183..09ff83e 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -9,6 +9,7 @@ src/gtk-helpers/event_config_dialog.c
> src/gui-wizard-gtk/main.c
> src/gui-wizard-gtk/wizard.c
> src/gui-wizard-gtk/wizard.glade
> +src/lib/create_dump_dir.c
> src/lib/event_config.c
> src/lib/parse_options.c
> src/lib/curl.c
> diff --git a/src/include/problem_data.h b/src/include/problem_data.h
> index 9bf46f5..c90f4a1 100644
> --- a/src/include/problem_data.h
> +++ b/src/include/problem_data.h
> @@ -35,6 +35,7 @@ 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 {
> @@ -61,6 +62,7 @@ void add_basics_to_problem_data(problem_data_t *pd);
>
> static inline void free_problem_data(problem_data_t *problem_data)
> {
> + //TODO: leaks problem item;
> if (problem_data)
> g_hash_table_destroy(problem_data);
> }
> diff --git a/src/include/report.h b/src/include/report.h
> index 3735ef6..21557ac 100644
> --- a/src/include/report.h
> +++ b/src/include/report.h
> @@ -39,6 +39,12 @@ 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.
> @@ -49,6 +55,8 @@ int report_problem_in_memory(problem_data_t *pd, int
> flags); /* 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 --git a/src/lib/create_dump_dir.c b/src/lib/create_dump_dir.c
> index 137da12..a089a3b 100644
> --- a/src/lib/create_dump_dir.c
> +++ b/src/lib/create_dump_dir.c
> @@ -19,6 +19,8 @@
>
> #include "internal_libreport.h"
>
> +#define NEW_PD_SUFFIX ".new"
> +
> static struct dump_dir *try_dd_create(const char *base_dir_name, const char
> *dir_name) {
> char *path = concat_path_file(base_dir_name, dir_name);
> @@ -88,3 +90,95 @@ struct dump_dir
> *create_dump_dir_from_problem_data(problem_data_t *problem_data,
>
> return dd;
> }
> +
> +LibreportError save_dump_dir_from_problem_data(problem_data_t
> *problem_data, char **problem_id, const char *base_dir_name) +{
> + char *type = get_problem_item_content_or_NULL(problem_data, "type");
Shouldn't we use FILENAME_TYPE instead of a string literal?
> +
> + if(!type)
> + {
> + error_msg(_("Missing required item: 'type'"));
> + return LR_MISSING_ITEM;
> + }
> +
> + char *time_s = get_problem_item_content_or_NULL(problem_data, "time");
Shouldn't we use FILENAME_TIME instead of a string literal?
> + if(!time_s)
> + {
> + /* 64 is a randomly picked constant which should be
> + * enough to hold the time in seconds for a long time..
> + */
> + char buf[64];
You known that you are casting the t variable to long unsigned, so why not to
set buf's size to 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);
> + add_to_problem_data_ext(problem_data, "time", buf, CD_FLAG_TXT);
> + }
> +
> + char dir_name[strlen(type) +
> sizeof("-"LIBREPORT_ISO_DATE_STRING_SAMPLE"-%lu") + sizeof(long)*3]; +
> *problem_id = (char *)xmalloc(sizeof(dir_name));
> + snprintf(*problem_id, sizeof(dir_name), "%s-%s-%lu"NEW_PD_SUFFIX, type,
> iso_date_string(NULL), (long)getpid()); + strncpy(dir_name, *problem_id,
> sizeof(dir_name));
I guess, xasprintf() could be used here. It is not necessary to use an extra
variable dir_name. (please, see line 167)
> +
> + VERB2 log("Saving to %s/%s", base_dir_name, *problem_id);
> + struct dump_dir *dd = try_dd_create(base_dir_name, dir_name);
> + if (!dd)
> + {
> + perror_msg("Can't create problem directory");
> + *problem_id = NULL;
Leaking problem_id
> + return LR_ERROR;
> + }
> +
> + GHashTableIter iter;
> + char *name;
> + struct problem_item *value;
> + 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)
> + {
> + char dest[PATH_MAX+1];
> + snprintf(dest, sizeof(dest), "%s/%s", 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);
> +
> + VERB2 log("copied %li bytes", (unsigned long)copied);
> +
> + continue;
> + }
> +
> + /* only files should contain '/' and those are handled earlier */
> + if (name[0] == '.' || strchr(name, '/'))
> + {
> + error_msg("Problem data field name contains disallowed chars:
> '%s'", name); + continue;
> + }
> +
> +//FIXME: what to do with CD_FLAG_BINs??
> + if (value->flags & CD_FLAG_BIN)
> + continue;
> +
> + dd_save_text(dd, name, value->content);
> + }
> +
> +
> +
> + char *suffix = strstr(*problem_id, NEW_PD_SUFFIX);
> + if (suffix)
> + *suffix = '\0';
> +
> + 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)
> + {
> + *problem_id = NULL;
Leaking problem_id
> + return LR_ERROR;
> + }
> + free(new_path);
> +
> + dd_close(dd);
> +
> + return LR_OK;
> +}
Regards,
Jakub
