Thanks, few comments below.
On Fri, 2012-04-20 at 11:04 +0200, Denys Vlasenko wrote:
On 03/21/2012 04:10 PM, Vratislav Podzimek wrote:
> ---
> src/daemon/abrt-server.c | 147 ++++++++++++++--------------------------------
> 1 files changed, 44 insertions(+), 103 deletions(-)
>
> diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c
> index 15f3370..c883452 100644
> --- a/src/daemon/abrt-server.c
> +++ b/src/daemon/abrt-server.c
> @@ -77,29 +77,21 @@ static unsigned total_bytes_read = 0;
> static uid_t client_uid = (uid_t)-1L;
>
> static int pid;
> -static char *executable;
> -static char *backtrace;
> -/* "python", "ruby" etc. */
> -static char *analyzer;
> -/* Directory base name: "pyhook", "ruby" etc. */
> -static char *dir_basename;
> -/* Crash reason.
> - * Python example:
> - * "CCMainWindow.py:1:<module>:ZeroDivisionError: integer division or
modulo by zero"
> - */
> -static char *reason;
> -
>
> /* Create a new debug dump from client session.
> * Caller must ensure that all fields in struct client
> * are properly filled.
> */
> -static int create_debug_dump()
> +static int create_debug_dump(GHashTable *problem_info)
> {
> /* Create temp directory with the debug dump.
> This directory is renamed to final directory name after
> all files have been stored into it.
> */
> + gchar *dir_basename = g_hash_table_lookup(problem_info, "basename");
> + GHashTableIter iter;
> + gpointer gpkey, gpvalue;
> +
> char *path = xasprintf("%s/%s-%s-%u.new",
> g_settings_dump_location,
> dir_basename,
We used to not allow creation of new problem reports
without basename (look for code which used to emit
"Some data are missing. Aborting" message).
Now you allow it: the directory name would be "(null)-DATETIME-PID".
This is probably not a good idea.
No I don't. Look at PATCH 3/4.
> @@ -115,15 +107,17 @@ static int create_debug_dump()
> }
> dd_create_basic_files(dd, client_uid);
>
> - dd_save_text(dd, FILENAME_ANALYZER, analyzer);
> - dd_save_text(dd, FILENAME_EXECUTABLE, executable);
> - dd_save_text(dd, FILENAME_BACKTRACE, backtrace);
> - dd_save_text(dd, FILENAME_REASON, reason);
> -
> - /* Obtain and save the command line. */
> - char *cmdline = get_cmdline(pid);
> - dd_save_text(dd, FILENAME_CMDLINE, cmdline ? : "");
> - free(cmdline);
> + gpkey = g_hash_table_lookup(problem_info, FILENAME_CMDLINE);
> + if(!gpkey)
Please follow the style: "if (!gpkey)"
> + {
> + /* Obtain and save the command line. */
> + char *cmdline = get_cmdline(pid);
> + if (cmdline)
> + {
> + dd_save_text(dd, FILENAME_CMDLINE, cmdline ? : "");
cmdline ? : "" is superfluous: you know that cmdline != NULL.
> + free(cmdline);
> + }
> + }
>
> /* Store id of the user whose application crashed. */
> char uid_str[sizeof(long) * 3 + 2];
> @@ -132,6 +126,12 @@ static int create_debug_dump()
>
> dd_save_text(dd, "abrt_version", VERSION);
>
> + g_hash_table_iter_init(&iter, problem_info);
> + while (g_hash_table_iter_next(&iter,&gpkey,&gpvalue))
More style violations.
> +/* Handles a message received from client over socket. */
> +static void process_message(GHashTable *problem_info, char *message)
> {
> + gchar *position;
> + gchar *key, *value;
>
> - if (strlen(contents)> max_len)
> + position = strchr(message, '=');
> + if (position)
> {
> + key = xstrndup(message, position - message);
> + value = g_ascii_strdown(key, strlen(key));
> + g_hash_table_insert(problem_info, key, value);
Bug? You insert (key, lowercased_key) pair, not (key, value) pair?
I want to make
the new code backward compatible, so this is needed. The
protocol has been looking like this -- "BASENAME=something" that has
been resulting in problem data containing a key-value pair ("basename",
"something").
> @@ -427,16 +372,12 @@ static int perform_http_xact(void)
> error_msg_and_die("Message is too long, aborting");
> }
>
> - /* Creates debug dump if all fields were already provided. */
> - if (!pid || !backtrace || !executable
> - || !analyzer || !dir_basename || !reason
> - ) {
> - error_msg_and_die("Some data are missing. Aborting");
> - }
> -
I think this check should stay (in a different form, of course).
PATCH 3/4 again.
--
Vratislav Podzimek <vpodzime(a)redhat.com>