[Bug 40695] Expose more caps code to plugins

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=40695

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2011-09-26 08:10:35 PDT ---
review of data-forms:


+  /* data forms provided via UpdateCapabilities()
+   * gchar * (client name) => GPtrArray<owned WockyDataForm> */
+  GHashTable *client_data_forms;

+  priv->client_data_forms = g_hash_table_new_full (g_str_hash, g_str_equal,
+      g_free, (GDestroyNotify) g_object_unref);

Either the documentation for the hash table's values, or the destroy function,
is wrong.

Ah later you changed this:

-      g_free, (GDestroyNotify) g_object_unref);
+      g_free, (GDestroyNotify) g_ptr_array_unref);

Okay then.

+              DEBUG ("client %s contributes data forms:", client_name);
+
+              for (j = 0; j < data_forms->len; j++)
+                {
+                  WockyDataForm *form = g_ptr_array_index (data_forms, j);
+                  DEBUG ("  %s", wocky_data_form_get_title (form));

Do you really want _get_title()? Does that give a meaningful description? Would
the value of the FORM_TYPE field be better?

+  /* just borrow the ref, data_forms doesn't have a free func */
+  while (g_hash_table_iter_next (&iter, &k, &v))
+    tp_g_ptr_array_extend (data_forms, v);

At this point (or possibly sooner if we can), we should verify that all forms
have a FORM_TYPE field, and that they are all distinct. From XEP-0115 §5.4
<http://xmpp.org/extensions/xep-0115.html#ver-proc>:

4. If the response includes more than one extended service discovery
information form with the same FORM_TYPE or the FORM_TYPE field contains more
than one <value/> element with different XML character data, consider the
entire response to be ill-formed.
5. If the response includes an extended service discovery information form
where the FORM_TYPE field is not of type "hidden" or the form does not include
a FORM_TYPE field, ignore the form but continue processing.

(Hmm interesting, do we obey clause 5?) But we should warn if a plugin (or
indeed Gabble itself) is causing us to generate ill-formed disco responses per
clause 4.

It turns out that a subsequent patch stops Gabble crashing if it can't hash its
own caps—but we should validate the forms sooner so we know who's to blame.
Right now we'll just get "Unable to fill in caps on presence stanza" in the
debug log, at most.

I don't think making gabble_connection_fill_in_caps able to fail is the correct
solution here. By the time we get that far, everything should be validated.

+  forms = info->data_forms;
+  if (data_forms != NULL)
+    info->data_forms = g_ptr_array_ref (data_forms);
+  if (forms != NULL)
+    g_ptr_array_unref (forms);

This feels a bit contorted, and I think it's wrong: in the data_forms == NULL
but info->data_forms != NULL path, info->data_forms will end up pointing to
garbage. How about:

  tp_clear_pointer (&info->data_forms, g_ptr_array_unref);
  if (data_forms != NULL)
    info->data_forms = g_ptr_array_ref (data_forms);

Hmm, perhaps you're trying to guard against the case where data_forms ==
info->data_forms (in which case my code there would be wrong)? Ugh C. This code
appears twice. I wonder if code could be shared between
gabble_presence_cache_add_own_caps and capability_info_recvd.


   if (node == NULL)
-    features = gabble_presence_peek_caps (self->self_presence);
+    {
+      features = gabble_presence_peek_caps (self->self_presence);
+      data_forms = gabble_presence_peek_data_forms (self->self_presence);
   /* If node is not NULL, it can be either a caps bundle as defined in the
    * legacy XEP-0115 version 1.3 or an hash as defined in XEP-0115 version
    * 1.5. Let's see if it's a verification string we've told the cache about.
    */
+    }

This is some funky placement of that closing brace: the comment refers to the
'else' block, not the 'if' block you've placed it inside.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.