On 07/10/2012 06:07 PM, Marcel Holtmann wrote:
Hi Jukka,
+struct connman_config_item **connman_config_get_configs(void)
+{
+ GHashTableIter iter_file, iter_config;
+ gpointer value, key;
+ struct connman_config_item **configs = NULL;
+ int i, count = 0;
+
+ g_hash_table_iter_init(&iter_file, config_table);
+ while (g_hash_table_iter_next(&iter_file, &key, &value) == TRUE) {
+ struct connman_config *config_file = value;
+
+ g_hash_table_iter_init(&iter_config,
+ config_file->service_table);
+ while (g_hash_table_iter_next(&iter_config, &key,
+ &value) == TRUE) {
+ struct connman_config_service *config = value;
+ configs = g_try_realloc(configs, (count + 1) *
+ sizeof(struct connman_config_item *));
+ if (configs == NULL)
+ return NULL;
Why are we doing this? We know the size of hash table. So allocate this
ahead of time,
We need to traverse two hashes so it complicates the issue. I changed
the code but IMHO the code looks now horrible.
not sure if it can get more bad. Lets try and then we see.
+
+ configs[count] =
+ g_try_new0(struct connman_config_item, 1);
+ if (configs[count] == NULL)
+ goto cleanup;
And why this? Is not something simple like
g_try_new0(struct connman_config_item, count)
sufficient?
We have already allocated the array where the pointers should be, so it
makes it much simpler to allocate the individual structs one by one.
I do not see how this can be better. If you know how much to allocate,
then just allocate the struct space as well. The structs are all same
size, so I don't see any benefit of all these memory allocations.
I tried to implement what you suggest but realized that something like
this results in double free.
This is what I tried to do:
int i = 0, count;
g_hash_table_iter_init(&iter_file, config_table);
while (g_hash_table_iter_next(&iter_file, &key, &value) == TRUE) {
struct connman_config *config_file = value;
struct connman_config_entry *entry;
count = g_hash_table_size(config_file->service_table);
entries = g_try_realloc(entries, (i + count + 1) *
sizeof(struct connman_config_entry *));
if (entries == NULL)
return NULL;
entry = g_try_new0(struct connman_config_entry, count);
if (entry == NULL)
goto cleanup;
- so we allocate the array of structs and populate them in the next
while() loop
count = 0;
g_hash_table_iter_init(&iter_config,
config_file->service_table);
while (g_hash_table_iter_next(&iter_config, &key,
&value) == TRUE) {
struct connman_config_service *config = value;
entries[i] = &entry[count++];
- and then set the pointers
entries[i]->ident = g_strdup(config->ident);
entries[i]->name = g_strdup(config->name);
entries[i]->ssid = g_try_malloc0(config->ssid_len + 1);
if (entries[i]->ssid == NULL)
goto cleanup;
memcpy(entries[i]->ssid, config->ssid,
config->ssid_len);
entries[i]->ssid_len = config->ssid_len;
entries[i]->hidden = config->hidden;
i++;
}
Then in the array freeing function we do
void connman_config_free_entries(struct connman_config_entry **entries)
{
int i;
if (entries == NULL)
return;
for (i = 0; entries[i]; i++) {
g_free(entries[i]->ident);
g_free(entries[i]->name);
g_free(entries[i]->ssid);
g_free(entries[i]);
}
g_free(entries);
return;
}
This all works just fine if there is only one config entry in config
file (the count will be 1 in above). If there is more than one elements
(count > 1), then we allocate an array of structs in g_try_new0().
During the freeing phase the free of the first element of the array will
also free all the other elements. This will cause double free in
connman_config_free_entries().
Cheers,
Jukka
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
