Attachment 19246 Details for Bug 17329 – allow hash tables to contain complex types

[patch] allow hash tables to contain complex types

0006-allow-hash-tables-to-contain-complex-types.patch (text/plain), 11.77 KB, created by Robert McQueen on 2008-09-26 09:13:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Robert McQueen

Created: 2008-09-26 09:13:20 UTC

Size: 11.77 KB

patch

obsolete

>From dbcd873b7a24e1e372ea2c0ce50fe9ff72aab811 Mon Sep 17 00:00:00 2001
>From: Robert McQueen <robert.mcqueen@collabora.co.uk>
>Date: Tue, 23 Sep 2008 23:42:02 +0300
>Subject: [PATCH] allow hash tables to contain complex types
>
>Previously, dbus-glib has provided destroy functions for the keys and
>values when constructing hash tables, so that any hash tables it
>constructed could be entirely freed (along with their contents) by
>destroying/unreffing. Unfortunately this meant that any "complex" types,
>where you need to know the GType in order to free them, were not allowed
>in hash tables. In real terms, this was anything which dbus-glib
>marshalled to a GPtrArray, so any array of arrays, variants, structures,
>object paths, or other boxed types were not allowed as hash values.
>
>This patch allows a broader range of types to be used as the values in
>hash tables, including those where no simple free function is available.
>Instead of relying on the key/value destroy functions, the new hash_free
>function uses g_hash_table_foreach_steal to remove the keys and values
>pairwise and free them when the type is known.
>
>Unfortunately, it's part of the API assumptions that hash tables which
>were produced through the current API had valid free functions, and
>particularly that if the hash table was reffed by the application, that
>the keys/values would persist beyond when dbus-glib had unreffed it, and
>be freed when the hash table was later destroyed. So it's not sufficient
>to use only this new freeing method on all hash tables from now on - we
>have to behave in the old way for all of the previously allowable types
>(including any hash tables which contain any hash tables which were
>freeable in the old way).
>
>However, as these new hash tables contain values which will not be freed
>if you manipulate the hash table directly (removing or replacing keys,
>or destroying or unreffing it directly), and g_boxed_free should be used
>instead, a false free function is provided to print a critical warning
>for the developer in the case that memory would be leaked.
>---
> dbus/dbus-gvalue-utils.c        |  151 +++++++++++++++++++++++++++++++++++++--
> test/core/test-dbus-glib.c      |   37 ++++++----
> test/core/test-service-glib.xml |    4 +-
> 3 files changed, 168 insertions(+), 24 deletions(-)
>
>diff --git a/dbus/dbus-gvalue-utils.c b/dbus/dbus-gvalue-utils.c
>index 6e50be1..55f58b9 100644
>--- a/dbus/dbus-gvalue-utils.c
>+++ b/dbus/dbus-gvalue-utils.c
>@@ -293,7 +293,10 @@ unset_and_free_g_value (gpointer val)
> }
> 
> static gboolean
>-hash_free_from_gtype (GType gtype, GDestroyNotify *func)
>+gtype_can_simple_free (GType type);
>+
>+static gboolean
>+hash_simple_free_from_gtype (GType gtype, GDestroyNotify *func)
> {
>   switch (gtype)
>     {
>@@ -347,6 +350,19 @@ hash_free_from_gtype (GType gtype, GDestroyNotify *func)
>       else if (dbus_g_type_is_map (gtype))
>         {
>           const DBusGTypeSpecializedMapVtable* vtable;
>+          GType key_gtype, value_gtype;
>+
>+          key_gtype = dbus_g_type_get_map_key_specialization (gtype);
>+          value_gtype = dbus_g_type_get_map_value_specialization (gtype);
>+
>+          /* if either the key or the value don't have "simple" (without a
>+           * GType) free functions, then the hashtable's contents must be freed
>+           * with hashtable_free, so the hashtable itself can't have a simple
>+           * free function. */
>+          if (!gtype_can_simple_free (key_gtype) ||
>+              !gtype_can_simple_free (value_gtype))
>+            return FALSE;
>+
>           vtable = dbus_g_type_map_peek_vtable (gtype);
>           if (vtable->base_vtable.simple_free_func)
>             {
>@@ -368,6 +384,13 @@ hash_free_from_gtype (GType gtype, GDestroyNotify *func)
>     }
> }
> 
>+static gboolean
>+gtype_can_simple_free (GType type)
>+{
>+  GDestroyNotify func;
>+  return hash_simple_free_from_gtype (type, &func);
>+}
>+
> gboolean
> _dbus_gtype_is_valid_hash_key (GType type)
> {
>@@ -376,10 +399,24 @@ _dbus_gtype_is_valid_hash_key (GType type)
> }
> 
> gboolean
>-_dbus_gtype_is_valid_hash_value (GType type)
>+_dbus_gtype_is_valid_hash_value (GType gtype)
> {
>-  GDestroyNotify func;
>-  return hash_free_from_gtype (type, &func);
>+  /* anything we can take into a GValue using gvalue_take_hash_value is fine */
>+  switch (g_type_fundamental (gtype))
>+    {
>+    case G_TYPE_CHAR:
>+    case G_TYPE_UCHAR:
>+    case G_TYPE_BOOLEAN:
>+    case G_TYPE_INT:
>+    case G_TYPE_UINT:
>+    case G_TYPE_DOUBLE:
>+    case G_TYPE_STRING:
>+    case G_TYPE_BOXED:
>+    case G_TYPE_OBJECT:
>+      return TRUE;
>+    }
>+
>+  return FALSE;
> }
> 
> GHashFunc
>@@ -417,13 +454,45 @@ _dbus_g_hash_equal_from_gtype (GType gtype)
>     }
> }
> 
>+static void
>+hash_fake_simple_free_func (gpointer val)
>+{
>+  /* Havoc would be proud... :P */
>+  g_critical ("If you see this message then the author of this application or "
>+      "one of its libraries has tried to remove or replace the value %p in a "
>+      "hash table which was constructed by the D-Bus Glib bindings.\n\n"
>+
>+      "However, it was not possible for the bindings to provide a destroy "
>+      "function to g_hash_table_new_full which is able to free this value, as "
>+      "its GType must be known in order to free it. This means the memory "
>+      "allocated to store the value has most likely just been leaked.\n\n"
>+
>+      "To avoid this error, the GHashTable (or keys/values \"stolen\" from "
>+      "it) should be freed by using g_boxed_free as follows:\n"
>+      "  g_boxed_free (dbus_g_type_get_map (\"GHashTable\", key_gtype, "
>+      "value_gtype), hash_table);\n", val);
>+}
>+
> GDestroyNotify
> _dbus_g_hash_free_from_gtype (GType gtype)
> {
>   GDestroyNotify func;
>   gboolean ret;
>-  ret = hash_free_from_gtype (gtype, &func);
>-  g_assert (ret != FALSE);
>+
>+  ret = hash_simple_free_from_gtype (gtype, &func);
>+
>+  /* if the value doesn't have a simple free function, we cannot define a
>+   * meaningful free function here. instead, this hash table must be freed
>+   * using g_boxed_free so that the hash_free function gets invoked. if the
>+   * user does not do so, we provide a fake free function to provide a warning
>+   * in this case. */
>+  if (ret == FALSE)
>+    {
>+      g_assert (_dbus_gtype_is_valid_hash_value (gtype));
>+
>+      func = hash_fake_simple_free_func;
>+    }
>+
>   return func;
> }
> 
>@@ -618,12 +687,80 @@ hashtable_copy (GType type, gpointer src)
>   return ret;
> }
> 
>+/* we leave this here for backwards compatibility - any hash tables nested
>+ * inside hash tables will use this as their free function if users were
>+ * already relying on it, but dbus-glib itself will never use it directly as
>+ * hashtable_free is also defined. */
> static void
> hashtable_simple_free (gpointer val)
> {
>   g_hash_table_unref (val);
> }
> 
>+struct DBusGHashTableFreeData
>+{
>+  GType key_gtype;
>+  GType value_gtype;
>+};
>+
>+static gboolean
>+hashtable_free_foreach_steal (gpointer key,
>+                              gpointer value,
>+                              gpointer user_data)
>+{
>+  struct DBusGHashTableFreeData *data = user_data;
>+  GValue val = { 0, };
>+
>+  g_value_init (&val, data->key_gtype);
>+  gvalue_take_hash_value (&val, key);
>+  g_value_unset (&val);
>+
>+  g_value_init (&val, data->value_gtype);
>+  gvalue_take_hash_value (&val, value);
>+  g_value_unset (&val);
>+
>+  return TRUE;
>+}
>+
>+static void
>+hashtable_free (GType type,
>+                gpointer val)
>+{
>+  struct DBusGHashTableFreeData data = { 0, };
>+  GHashTable *hash = val;
>+
>+  data.key_gtype = dbus_g_type_get_map_key_specialization (type);
>+  data.value_gtype = dbus_g_type_get_map_value_specialization (type);
>+
>+  /* wheee, fun. two cases here. either:
>+   *
>+   * a) the keys and value types both have simple (ie, no * GType parameter is
>+   * needed to know how to free them) free functions, in which case they were
>+   * set as the hash free functions when the hash table was constructed.  in
>+   * this case, it's sufficient for us to unref the hash table as before. we
>+   * have to keep doing this in order to maintain compatibility with the ABI
>+   * which was around before hash tables could contain types which don't have
>+   * simple free functions (such as GPtrArrays of other stuff). for these
>+   * tables, users were able to ref the hash tables and add/remove values, and
>+   * rely on meaningful free functions.
>+   *
>+   * b) for any other key or value types where /do/ need to know the GType in
>+   * order to free it, this function is the only "right" way to free the hash
>+   * table. both the key and value free functions were set to print a big nasty
>+   * warning, and we free the contents of the hashtable with foreach_steal.
>+   */
>+  if (gtype_can_simple_free (data.key_gtype) &&
>+      gtype_can_simple_free (data.value_gtype))
>+    {
>+      g_hash_table_unref (hash);
>+    }
>+  else
>+    {
>+      g_hash_table_foreach_steal (hash, hashtable_free_foreach_steal, &data);
>+      g_hash_table_unref (hash);
>+    }
>+}
>+
> static gpointer
> valuearray_constructor (GType type)
> {
>@@ -1043,7 +1180,7 @@ _dbus_g_type_specialized_builtins_init (void)
>   static const DBusGTypeSpecializedMapVtable hashtable_vtable = {
>     {
>       hashtable_constructor,
>-      NULL,
>+      hashtable_free,
>       hashtable_copy,
>       hashtable_simple_free,
>       NULL,
>diff --git a/test/core/test-dbus-glib.c b/test/core/test-dbus-glib.c
>index b813828..d659ff3 100644
>--- a/test/core/test-dbus-glib.c
>+++ b/test/core/test-dbus-glib.c
>@@ -1380,9 +1380,15 @@ main (int argc, char **argv)
>   {
>     GHashTable *table;
>     GHashTable *ret_table = NULL;
>-    const gchar *foo[] = { "foo", NULL };
>-    const gchar *bar[] = { "bar", "baz", NULL };
>-    const gchar **ret_foo = NULL, **ret_bar = NULL;
>+    GPtrArray *foo, *bar;
>+    GPtrArray *ret_foo = NULL, *ret_bar = NULL;
>+
>+    foo = g_ptr_array_new ();
>+    g_ptr_array_add (foo, "/foo");
>+
>+    bar = g_ptr_array_new ();
>+    g_ptr_array_add (bar, "/bar");
>+    g_ptr_array_add (bar, "/baz");
> 
>     table = g_hash_table_new (g_str_hash, g_str_equal);
>     g_hash_table_insert (table, "/foo", foo);
>@@ -1394,6 +1400,10 @@ main (int argc, char **argv)
>           &ret_table, &error))
>       lose_gerror ("Failed to complete DictOfObjs call", error);
> 
>+    g_ptr_array_free (foo, TRUE);
>+    g_ptr_array_free (bar, TRUE);
>+    g_hash_table_destroy (table);
>+
>     if (ret_table == NULL)
>       lose ("DictOfObjs didn't return a hash table");
> 
>@@ -1406,20 +1416,17 @@ main (int argc, char **argv)
>     if (ret_foo == NULL || ret_bar == NULL)
>       lose ("DictOfObjs is missing entries");
> 
>-    if (ret_foo[0] == NULL ||
>-        ret_foo[1] != NULL ||
>-        strcmp (ret_foo[0], "foo") != 0)
>-      lose ("DictOfObjs mangled foo");
>+    if (ret_foo->len != 1 ||
>+        strcmp (g_ptr_array_index (ret_foo, 0), "/foo") != 0)
>+      lose ("DictOfObjs mangled /foo");
> 
>-    if (ret_bar[0] == NULL ||
>-        ret_bar[1] == NULL ||
>-        ret_bar[2] != NULL ||
>-        strcmp (ret_bar[0], "bar") != 0 ||
>-        strcmp (ret_bar[1], "baz") != 0)
>-      lose ("DictOfObjs mangled bar");
>+    if (ret_bar->len != 2 ||
>+        strcmp (g_ptr_array_index (ret_bar, 0), "/bar") != 0 ||
>+        strcmp (g_ptr_array_index (ret_bar, 1), "/baz") != 0)
>+      lose ("DictOfObjs mangled /bar");
> 
>-    g_hash_table_destroy (table);
>-    g_hash_table_destroy (ret_table);
>+    g_boxed_free (dbus_g_type_get_map ("GHashTable", DBUS_TYPE_G_OBJECT_PATH,
>+          dbus_g_type_get_collection ("GPtrArray", DBUS_TYPE_G_OBJECT_PATH)), ret_table);
> 
>     g_mem_profile ();
>   }
>diff --git a/test/core/test-service-glib.xml b/test/core/test-service-glib.xml
>index b635731..216b58a 100644
>--- a/test/core/test-service-glib.xml
>+++ b/test/core/test-service-glib.xml
>@@ -150,8 +150,8 @@
> 
>     <method name="DictOfObjs">
>       <annotation name="org.freedesktop.DBus.GLib.Async" value=""/>
>-      <arg type="a{oas}" direction="in" />
>-      <arg type="a{oas}" direction="out" />
>+      <arg type="a{oao}" direction="in" />
>+      <arg type="a{oao}" direction="out" />
>     </method>
> 
>     <method name="EmitFrobnicate">
>-- 
>1.5.4.4
>

Actions: View | Splinter Review

Attachments on bug 17329: 19246

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.