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.
>
>
> > @@ -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 can see it now, sorry for the previous comment. I had it changed on my
system, but didn't generate new files for git send-email. This is why I
was replying about lower-casing and not about that bug.
>
>
> > @@ -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).
>
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
