[Telepathy] [PATCH] tp-helpers

[Dafydd Harries]

Ar 13/12/2006 am 00:13, ysgrifennodd Xavier Claessens:
> Hi,
> 
> tp-helpers.[ch] in libtelepathy is out-of-date and don't support
> new .manager files. Here is a patch to fix that. I use the new API with
> gossip and it seems to work pretty well.
> 
> Xavier Claessens.

Hi Xavier,

Your patch looks very good overall, just a few thing I'm concerned about;
mainly concerned with handling invalid input/being future-proof.

> +  cm_info = g_new0(TpConnMgrInfo, 1);

You could slice-allocate these structures, though it's probably not a
significant benefit.

> @@ -173,9 +170,7 @@ TpConnMgrInfo *tp_connmgr_get_info(gchar
>      g_printerr("%s", error->message);
>      g_error_free(error);
>      g_key_file_free(file);
> -    g_free(absolute_filepath);
> -    g_free(cm_info->name);
> -    free(cm_info);
> +    tp_connmgr_info_free(cm_info);
>      return NULL;
>    }
>    if (!(cm_info->object_path = g_key_file_get_string(file,CM_CONF_GROUP,

I like the introduction of tp_connmgr_info_free, it makes things much
clearer.

> +  for(group = groups+1; *group; group++)
>    {

This code seems to assume that all groups after the first one are protocol
declarations. I would prefer that you actually check that the group name
starts with "Protocol ".

> +    params = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +                                   (GDestroyNotify)_prot_param_free);
> +    keys = g_strsplit(*group, " ", 2);
> +    prot_name = g_strdup(keys[1]);

There's no guarantee that there are actually two elements in the array at this
point (if the group name doesn't have a space in it, there will only be one).

> +      strv = g_strsplit(*key, "-", 2);
> +      param_name = g_strdup(strv[1]);

Again, doesn't handle the case where there is no "-" in the key name.

> +      param = g_hash_table_lookup(params, param_name);
> +      if (!param)
> +      {
> +        param = g_new0(TpConnMgrProtParam, 1);

This could also be slice-allocated.

>From a code structure point of view, the tp_connmgr_get_info function seems
very long. Perhaps the code to parse a single protocol group could be split
out into a separate function? This might make the error control flow clearer
too.

-- 
Dafydd