[PATCH] get_reporter_plugin_settings() returns GHashTable

[Nikola Pajkovsky]

static void get_reporter_plugin_settings(const vector_string_t&
        reporters, map_map_string_t &settings)

a new interface is

static GHashTable *get_reporter_plugin_settings(const vector_string_t&
        reporters)

Signed-off-by: Nikola Pajkovsky 
---
 src/cli/dbus.cpp    |    6 ++--
 src/cli/dbus.h      |    3 +-
 src/cli/report.cpp  |   65 +++++++++++++++++++++++++++++++++++++-------------
 src/lib/abrt_dbus.h |   29 ++++++++++++++++++++++
 4 files changed, 82 insertions(+), 21 deletions(-)

diff --git a/src/cli/dbus.cpp b/src/cli/dbus.cpp
index 7565d5b..f9271b8 100644
--- a/src/cli/dbus.cpp
+++ b/src/cli/dbus.cpp
@@ -161,7 +161,7 @@ map_crash_data_t call_CreateReport(const char* crash_id)

report_status_t call_Report(const map_crash_data_t& report,
    		    const vector_string_t& reporters,
-			    const map_map_string_t &plugins)
+			    GHashTable *plugins)
 {
     DBusMessage* msg = new_call_msg(__func__ + 5);
     DBusMessageIter out_iter;
@@ -172,8 +172,8 @@ report_status_t call_Report(const map_crash_data_t& report,
     /* parameter #2: reporters to use */
     store_val(&out_iter, reporters);
     /* parameter #3 (opt): plugin config */
-    if (!plugins.empty())
-        store_val(&out_iter, plugins);
+    if (g_hash_table_size(plugins))
+        store_hash_table_map_string_t(&out_iter, plugins);

DBusMessage *reply = send_get_reply_and_unref(msg);

diff --git a/src/cli/dbus.h b/src/cli/dbus.h
index 9c99c66..b837869 100644
--- a/src/cli/dbus.h
+++ b/src/cli/dbus.h
@@ -18,6 +18,7 @@
 #ifndef ABRT_CLI_DBUS_H
 #define ABRT_CLI_DBUS_H

+#include <glib.h>
 #include "abrt_dbus.h"
 #include "abrt_crash_dump.h"

@@ -41,7 +42,7 @@ map_crash_data_t call_CreateReport(const char *crash_id);
  */
 report_status_t call_Report(const map_crash_data_t& report,
    		    const vector_string_t& reporters,
-			    const map_map_string_t &plugins);
+			    GHashTable *plugins);

int32_t call_DeleteDebugDump(const char* crash_id);

diff --git a/src/cli/report.cpp b/src/cli/report.cpp
index 78a3891..ff953b2 100644
--- a/src/cli/report.cpp
+++ b/src/cli/report.cpp
@@ -503,23 +503,49 @@ static bool set_echo(bool enabled)
 }

/**
+ * Destroy plugin settings
+ * @param ht
+ *      Plugin settings in pattern GHashTable<char *, map_string_t *>
+ */
+static void destroy_plugin_settings(GHashTable *ht)
+{
+    GHashTableIter iter;
+    gpointer key, value;
+
+    g_hash_table_iter_init (&iter, ht);
+    while (g_hash_table_iter_next (&iter, &key, &value))
+    {
+        free(key);
+        delete (map_string_t *)value;
+    }
+
+    g_hash_table_destroy(ht);
+}
+
+
+/**
  * Gets reporter plugin settings.
  * @param reporters
  *   List of reporter names. Settings of these reporters are handled.
- * @param settings
+ * @return settings
  *   A structure filled with reporter plugin settings.
+ *   It's GHashTable<char *, map_plugin_t *> and must be passed to
+ *   destroy_plugin_settings()
  */
-static void get_reporter_plugin_settings(const vector_string_t& reporters,
-        map_map_string_t &settings)
+static GHashTable *get_reporter_plugin_settings(const vector_string_t& reporters)
 {
     /* First of all, load system-wide report plugin settings. */
+    GHashTable *settings = g_hash_table_new(g_str_hash, g_str_equal);
+
     for (vector_string_t::const_iterator it = reporters.begin(); it != reporters.end(); ++it)
     {
-        map_string_t single_plugin_settings = call_GetPluginSettings(it->c_str());
+        map_string_t *single_plugin_settings =  new map_string_t;
+        *single_plugin_settings = call_GetPluginSettings(it->c_str());
+
         // Copy the received settings as defaults.
         // Plugins won't work without it, if some value is missing
         // they use their default values for all fields.
-        settings[it->c_str()] = single_plugin_settings;
+        g_hash_table_insert(settings, xstrdup(it->c_str()), (void*)single_plugin_settings);
     }

/* Second, load user-specific settings, which override
@@ -528,12 +554,16 @@ static void get_reporter_plugin_settings(const vector_string_t& reporters,
     const char* homedir = pw ? pw->pw_dir : NULL;
     if (homedir)
     {
-        map_map_string_t::const_iterator itend = settings.end();
-        for (map_map_string_t::iterator it = settings.begin(); it != itend; ++it)
+        GHashTableIter iter;
+        gpointer key, value;
+
+        g_hash_table_iter_init (&iter, settings);
+        while (g_hash_table_iter_next (&iter, &key, &value))
         {
             map_string_t single_plugin_settings;
             std::string path = std::string(homedir) + "/.abrt/"
-                + it->first + ".conf";
+                + (char*)key + ".conf";
+
             /* Load plugin config in the home dir. Do not skip lines with empty value (but containing a "key="),
                because user may want to override password from /etc/abrt/plugins/*.conf, but he prefers to
                enter it every time he reports. */
@@ -543,9 +573,10 @@ static void get_reporter_plugin_settings(const vector_string_t& reporters,
             // Merge user's plugin settings into already loaded settings.
             map_string_t::const_iterator valit, valitend = single_plugin_settings.end();
             for (valit = single_plugin_settings.begin(); valit != valitend; ++valit)
-                it->second[valit->first] = valit->second;
+                (*(map_string_t*)value)[valit->first] = valit->second;
         }
     }
+    return settings;
 }

/**
@@ -628,8 +659,7 @@ int report(const char *crash_id, int flags)
     }

/* Get settings */
-    map_map_string_t reporters_settings;
-    get_reporter_plugin_settings(reporters, reporters_settings);
+    GHashTable *reporters_settings = get_reporter_plugin_settings(reporters);

int errors = 0;
     int plugins = 0;
@@ -661,18 +691,18 @@ int report(const char *crash_id, int flags)
                 continue;
             }

-            map_map_string_t::iterator settings = reporters_settings.find(it->c_str());
-            if (settings != reporters_settings.end())
+            map_string_t *settings = (map_string_t *)g_hash_table_lookup(reporters_settings, it->c_str());
+            if (settings)
             {
-                map_string_t::iterator rating_setting = settings->second.find("RatingRequired");
-                if (rating_setting != settings->second.end()
+                map_string_t::iterator rating_setting = settings->find("RatingRequired");
+                if (rating_setting != settings->end()
                         && string_to_bool(rating_setting->second.c_str())
                         && rating < 3)
                 {
                     puts(_("Reporting disabled because the backtrace is unusable"));

const char *package = get_crash_data_item_content_or_NULL(cr, FILENAME_PACKAGE);
-                    if (package[0])
+                    if (package && package[0])
                         printf(_("Please try to install debuginfo manually using the command: "debuginfo-install %s" and try again\n"), package);

plugins++;
@@ -688,7 +718,7 @@ int report(const char *crash_id, int flags)
                 continue;
             }

-            ask_for_missing_settings(it->c_str(), settings->second);
+            ask_for_missing_settings(it->c_str(), *settings);

vector_string_t cur_reporter(1, *it);
             report_status_t r = call_Report(cr, cur_reporter, reporters_settings);
@@ -701,6 +731,7 @@ int report(const char *crash_id, int flags)
         }
     }

+    destroy_plugin_settings(reporters_settings);
     printf(_("Crash reported via %d report events (%d errors)\n"), plugins, errors);
     return errors != 0;
 }
diff --git a/src/lib/abrt_dbus.h b/src/lib/abrt_dbus.h
index cdc963c..16b91d5 100644
--- a/src/lib/abrt_dbus.h
+++ b/src/lib/abrt_dbus.h
@@ -20,6 +20,7 @@
 #define ABRT_DBUS_H

#include <dbus/dbus.h>
+#include <glib.h>
 #include "abrtlib.h"

@@ -200,6 +201,34 @@ static inline void store_val(DBusMessageIter* iter, const std::vector<E>& val) {
 template<typename K, typename V>
 static inline void store_val(DBusMessageIter* iter, const std::map<K,V>& val)  { store_map(iter, val); }

+/* next patch will rewrite this into c */
+static inline void store_hash_table_map_string_t(DBusMessageIter* iter, GHashTable *ht)
+{
+    DBusMessageIter sub_iter;
+    if (!dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{sa{ss}}", &sub_iter))
+        die_out_of_memory();
+
+    GHashTableIter ht_iter;
+    gpointer key, value;
+
+    g_hash_table_iter_init(&ht_iter, ht);
+    while (g_hash_table_iter_next(&ht_iter, &key, &value))
+    {
+        DBusMessageIter sub_sub_iter;
+        if (!dbus_message_iter_open_container(&sub_iter, DBUS_TYPE_DICT_ENTRY, NULL, &sub_sub_iter))
+            die_out_of_memory();
+
+        store_val(&sub_sub_iter, (char *)key);
+        store_val(&sub_sub_iter, *(map_string_t *)value);
+
+        if (!dbus_message_iter_close_container(&sub_iter, &sub_sub_iter))
+            die_out_of_memory();
+    }
+
+    if (!dbus_message_iter_close_container(iter, &sub_iter))
+        die_out_of_memory();
+}
+

/*
  * Helpers for parsing DBus messages

[Denys Vlasenko]

On Wed, 2010-12-01 at 14:59 +0100, Nikola Pajkovsky wrote:
> static void get_reporter_plugin_settings(const vector_string_t&
>         reporters, map_map_string_t &settings)
>
> a new interface is
>
> static GHashTable *get_reporter_plugin_settings(const vector_string_t&
>         reporters)
> Signed-off-by: Nikola Pajkovsky
>
> src/cli/dbus.cpp    |    6 ++--
>  src/cli/dbus.h      |    3 +-
>  src/cli/report.cpp  |   65 +++++++++++++++++++++++++++++++++++++-------------
>  src/lib/abrt_dbus.h |   29 ++++++++++++++++++++++
>  4 files changed, 82 insertions(+), 21 deletions(-)
> diff --git a/src/cli/report.cpp b/src/cli/report.cpp
> index 78a3891..ff953b2 100644
> --- a/src/cli/report.cpp
> +++ b/src/cli/report.cpp
> @@ -503,23 +503,49 @@ static bool set_echo(bool enabled)
>  }
>
> /**
> Destroy plugin settings
> @param ht
>  Plugin settings in pattern GHashTable<char *, map_string_t *>
> */
>
> +static void destroy_plugin_settings(GHashTable *ht)

I would call it destroy_GHashTable_map_string_t()
or maybe free_GHashTable_map_string_t().
> */
> -static void get_reporter_plugin_settings(const vector_string_t& reporters,
>    map_map_string_t &settings)
>
> +static GHashTable *get_reporter_plugin_settings(const vector_string_t& reporters)
>  {
>      /* First of all, load system-wide report plugin settings. */
> GHashTable *settings = g_hash_table_new(g_str_hash, g_str_equal);

Alternatively, you may want to use g_hash_table_new_full here
http://library.gnome.org/devel/glib/stable/glib-Hash-Tables.html#g-hash-tabl...
- it allows "freeing" functions to be remembered in created GHashTable.
I think it means that simple g_hash_table_destroy() will be able to
fully free the created GHashTable.
>  for (vector_string_t::const_iterator it = reporters.begin(); it != reporters.end(); ++it)
>  {
>    map_string_t single_plugin_settings = call_GetPluginSettings(it->c_str());
>    map_string_t *single_plugin_settings =  new map_string_t;
>    *single_plugin_settings = call_GetPluginSettings(it->c_str());
>    // Copy the received settings as defaults.
>    // Plugins won't work without it, if some value is missing
>    // they use their default values for all fields.
>    settings[it->c_str()] = single_plugin_settings;
>    g_hash_table_insert(settings, xstrdup(it->c_str()), (void*)single_plugin_settings);

Here, if key already exists, it (and corresponding value) needs to be
properly freed.
This won't happen if you created GHashTable with simple g_hash_table_new
- the key and value will leak.
>    while (g_hash_table_iter_next (&iter, &key, &value))
>    {
>        map_string_t single_plugin_settings;
>        std::string path = std::string(homedir) + "/.abrt/"
>            + it->first + ".conf";
>            + (char*)key + ".conf";

You may also get rid of this C++ism (convert it to xasprintf)
while you are at it.