[ABRT PATCH] abrt-cli report: accept sha1 hashes of directory names. #693

[Denys Vlasenko]

Signed-off-by: Denys Vlasenko <dvlasenk-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
po/POTFILES.in | 1 +
src/cli/abrt-cli-core.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
src/cli/abrt-cli-core.h | 4 ++++
src/cli/list.c | 57 ++++--------------------------------------------
src/cli/report.c | 22 ++++++-------------
5 files changed, 74 insertions(+), 68 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index cd0a887..817b3fd 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -42,6 +42,7 @@ src/plugins/bodhi.c

src/hooks/abrt-merge-pstoreoops.c

+src/cli/abrt-cli-core.c
src/cli/abrt-cli.c
src/cli/list.c
src/cli/status.c
diff --git a/src/cli/abrt-cli-core.c b/src/cli/abrt-cli-core.c
index 26d08cc..6826a69 100644
--- a/src/cli/abrt-cli-core.c
+++ b/src/cli/abrt-cli-core.c
@@ -60,3 +60,61 @@ vector_of_problem_data_t *fetch_crash_infos(GList *dir_list)

return vpd;
}
+
+
+static bool isxdigit_str(const char *str)
+{
+ do
+ {
+ if (*str < '0' || *str > '9')
+ if ((*str|0x20) < 'a' || (*str|0x20) > 'f')
+ return false;
+ str++;
+ } while (*str);
+ return true;
+}
+
+char *str2hash(const char *str)
+{
+ static char result[SHA1_RESULT_LEN*2 + 1];
+
+ char hash_bytes[SHA1_RESULT_LEN];
+ sha1_ctx_t sha1ctx;
+ sha1_begin(&sha1ctx);
+ sha1_hash(&sha1ctx, str, strlen(str));
+ sha1_end(&sha1ctx, hash_bytes);
+ bin2hex(result, hash_bytes, SHA1_RESULT_LEN)[0] = '\0';
+ return result;
+}
+
+struct name_resolution_param {
+ const char *shortcut;
+ unsigned strlen_shortcut;
+ char *found_name;
+};
+static int find_dir_by_hash(struct dump_dir *dd, void *arg)
+{
+ struct name_resolution_param *param = arg;
+ char *h = str2hash(dd->dd_dirname);
+ if (strncasecmp(param->shortcut, h, param->strlen_shortcut) == 0)
+ {
+ if (param->found_name)
+ error_msg_and_die(_("'%s' identifies more than one problem directory"), param->shortcut);
+ param->found_name = xstrdup(dd->dd_dirname);
+ }
+ return 0;
+}
+
+char *hash2dirname(const char *hash)
+{
+ unsigned hash_len = strlen(hash);
+ if (!isxdigit_str(hash) || hash_len < 5)
+ return NULL;
+
+ /* Try loading by dirname hash */
+ struct name_resolution_param param = { hash, hash_len, NULL };
+ GList *dir_list = get_problem_storages();
+ for (GList *li = dir_list; li; li = li->next)
+ for_each_problem_in_dir(li->data, getuid(), find_dir_by_hash, &param);
+ return param.found_name;
+}
diff --git a/src/cli/abrt-cli-core.h b/src/cli/abrt-cli-core.h
index 8eedc13..d28ef95 100644
--- a/src/cli/abrt-cli-core.h
+++ b/src/cli/abrt-cli-core.h
@@ -30,4 +30,8 @@ void free_vector_of_problem_data(vector_of_problem_data_t *vector);
vector_of_problem_data_t *new_vector_of_problem_data(void);
vector_of_problem_data_t *fetch_crash_infos(GList *dir_list);

+char *str2hash(const char *str);
+char *hash2dirname(const char *hash);
+
+
#endif /* ABRT_CLI_CORE_H_ */
diff --git a/src/cli/list.c b/src/cli/list.c
index a257623..c6bc895 100644
--- a/src/cli/list.c
+++ b/src/cli/list.c
@@ -30,49 +30,6 @@
* ~/.abrt/spool and /var/tmp/abrt? needs more _meditation_.
*/

-static bool isxdigit_str(const char *str)
-{
- do
- {
- if (*str < '0' || *str > '9')
- if ((*str|0x20) < 'a' || (*str|0x20) > 'f')
- return false;
- str++;
- } while (*str);
- return true;
-}
-
-static char *str2hash(const char *str)
-{
- static char result[SHA1_RESULT_LEN*2 + 1];
-
- char hash_bytes[SHA1_RESULT_LEN];
- sha1_ctx_t sha1ctx;
- sha1_begin(&sha1ctx);
- sha1_hash(&sha1ctx, str, strlen(str));
- sha1_end(&sha1ctx, hash_bytes);
- bin2hex(result, hash_bytes, SHA1_RESULT_LEN)[0] = '\0';
- return result;
-}
-
-struct name_resolution_param {
- const char *shortcut;
- unsigned strlen_shortcut;
- char *found_name;
-};
-static int find_dir_by_hash(struct dump_dir *dd, void *arg)
-{
- struct name_resolution_param *param = arg;
- char *h = str2hash(dd->dd_dirname);
- if (strncasecmp(param->shortcut, h, param->strlen_shortcut) == 0)
- {
- if (param->found_name)
- error_msg_and_die(_("'%s' identifies more than one problem directory"), param->shortcut);
- param->found_name = xstrdup(dd->dd_dirname);
- }
- return 0;
-}
-
static problem_data_t *load_problem_data(const char *dump_dir_name)
{
/* First, try loading by dirname */
@@ -81,19 +38,13 @@ static problem_data_t *load_problem_data(const char *dump_dir_name)
struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ DD_OPEN_READONLY);
logmode = sv_logmode;

- unsigned name_len = strlen(dump_dir_name);
/* (git requires at least 5 char hash prefix, we do the same) */
- if (!dd && errno == ENOENT && isxdigit_str(dump_dir_name) && name_len >= 5)
+ if (!dd && errno == ENOENT)
{
/* Try loading by dirname hash */
- struct name_resolution_param param = { dump_dir_name, name_len, NULL };
- GList *dir_list = get_problem_storages();
- for (GList *li = dir_list; li; li = li->next)
- for_each_problem_in_dir(li->data, getuid(), find_dir_by_hash, &param);
- if (!param.found_name)
- return NULL;
- dd = dd_opendir(param.found_name, /*flags:*/ DD_OPEN_READONLY);
- free(param.found_name);
+ char *name2 = hash2dirname(dump_dir_name);
+ dd = dd_opendir(name2, /*flags:*/ DD_OPEN_READONLY);
+ free(name2);
}

if (!dd)
diff --git a/src/cli/report.c b/src/cli/report.c
index 781c691..c5787e0 100644
--- a/src/cli/report.c
+++ b/src/cli/report.c
@@ -51,26 +51,18 @@ int cmd_report(int argc, const char **argv)
{
const char *dir_name = *argv++;

- vector_of_problem_data_t *ci = NULL;
- if (*dir_name == '@')
+ char *free_me = NULL;
+ if (access(dir_name, F_OK) != 0 && errno == ENOENT)
{
- dir_name++;
- unsigned at = xatoi_positive(dir_name);
-
- ci = fetch_crash_infos(D_list);
- if (at >= ci->len)
- error_msg_and_die("error: number is out of range '%s'", dir_name);
-
- g_ptr_array_sort_with_data(ci, &cmp_problem_data, (char *) FILENAME_LAST_OCCURRENCE);
- problem_data_t *pd = get_problem_data(ci, at);
-
- dir_name = problem_data_get_content_or_NULL(pd, CD_DUMPDIR);
+ free_me = hash2dirname(dir_name);
+ if (free_me)
+ dir_name = free_me;
}
-
int status = report_problem_in_dir(dir_name,
LIBREPORT_WAIT
| LIBREPORT_RUN_CLI);
- free_vector_of_problem_data(ci);
+ free(free_me);
+
if (status)
exit(status);
}

[Jakub Filak]

Post by Denys Vlasenko
+char *str2hash(const char *str)
+{
+ static char result[SHA1_RESULT_LEN*2 + 1];
+
+ char hash_bytes[SHA1_RESULT_LEN];
+ sha1_ctx_t sha1ctx;
+ sha1_begin(&sha1ctx);
+ sha1_hash(&sha1ctx, str, strlen(str));
+ sha1_end(&sha1ctx, hash_bytes);
+ bin2hex(result, hash_bytes, SHA1_RESULT_LEN)[0] = '\0';
+ return result;
+}

I have nothing against returning a pointer to static data but I would
return it as 'const char *' to prevent situation where one may want to
free the return value (especially in case where the function have no
documentation)

Anyway, str2hash() is the fifth implementation of same functionality in
abrt project and I would moved it to some common library (libreport
xfuncs or so) and replace all existing implementations with str2hash()
function.

Other str2hex() implementors:
src/lib/kernel.c
src/plugins/abrt-action-analyze-backtrace.c
src/plugins/abrt-action-analyze-python.c
src/plugins/abrt-action-analyze-c.c

Post by Denys Vlasenko
+
+struct name_resolution_param {
+ const char *shortcut;
+ unsigned strlen_shortcut;
+ char *found_name;
+};

New line would be nice here.

Post by Denys Vlasenko
+static int find_dir_by_hash(struct dump_dir *dd, void *arg)
+{
+ struct name_resolution_param *param = arg;
+ char *h = str2hash(dd->dd_dirname);
+ if (strncasecmp(param->shortcut, h, param->strlen_shortcut) == 0)
+ {
+ if (param->found_name)
+ error_msg_and_die(_("'%s' identifies more than one problem directory"), param->shortcut);
+ param->found_name = xstrdup(dd->dd_dirname);
+ }
+ return 0;
+}
+
+char *hash2dirname(const char *hash)
+{
+ unsigned hash_len = strlen(hash);
+ if (!isxdigit_str(hash) || hash_len < 5)
+ return NULL;
+
+ /* Try loading by dirname hash */
+ struct name_resolution_param param = { hash, hash_len, NULL };
+ GList *dir_list = get_problem_storages();
+ for (GList *li = dir_list; li; li = li->next)
+ for_each_problem_in_dir(li->data, getuid(), find_dir_by_hash, &param);
+ return param.found_name;
+}
diff --git a/src/cli/abrt-cli-core.h b/src/cli/abrt-cli-core.h
index 8eedc13..d28ef95 100644
--- a/src/cli/abrt-cli-core.h
+++ b/src/cli/abrt-cli-core.h
@@ -30,4 +30,8 @@ void free_vector_of_problem_data(vector_of_problem_data_t *vector);
vector_of_problem_data_t *new_vector_of_problem_data(void);
vector_of_problem_data_t *fetch_crash_infos(GList *dir_list);
+char *str2hash(const char *str);
+char *hash2dirname(const char *hash);
+
+

I'd say that these functions deserve some documentation. At least how to
process the return values because return value of str2hash() must not be
freed but return value of hash2dirname() should be freed.

Post by Denys Vlasenko
#endif /* ABRT_CLI_CORE_H_ */

[Denys Vlasenko]

On 09/10/2013 11:24 AM, Jakub Filak wrote:
Anyway, str2hash() is the fifth implementation of same functionality in

Post by Jakub Filak
abrt project and I would moved it to some common library (libreport
xfuncs or so) and replace all existing implementations with str2hash()
function.
src/lib/kernel.c
src/plugins/abrt-action-analyze-backtrace.c
src/plugins/abrt-action-analyze-python.c
src/plugins/abrt-action-analyze-c.c

Good idea, we can do that as a separate patch.

Please review patch v2.

[Jakub Filak]

Post by Jakub Filak
Anyway, str2hash() is the fifth implementation of same functionality in

Post by Jakub Filak
abrt project and I would moved it to some common library (libreport
xfuncs or so) and replace all existing implementations with str2hash()
function.
src/lib/kernel.c
src/plugins/abrt-action-analyze-backtrace.c
src/plugins/abrt-action-analyze-python.c
src/plugins/abrt-action-analyze-c.c

Good idea, we can do that as a separate patch.

I've filed a ticket for it.

https://github.com/abrt/abrt/issues/694

Post by Jakub Filak
Please review patch v2.