From dbcd873b7a24e1e372ea2c0ce50fe9ff72aab811 Mon Sep 17 00:00:00 2001 From: Robert McQueen 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 @@ - - + + -- 1.5.4.4