[Bug 40695] Expose more caps code to plugins

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2011-09-27 07:11:48 PDT ---
(In reply to comment #1)
> +  /* 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>:
[...]
> (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.

What do you reckon we should do if a client is represented and gives some bad
data forms, or perhaps only one bad data form, or a duplicate form type? Do we
ignore *all* data forms from this client? Do we ignore all caps completely? I
can't decide what to do here.

> 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.

OK. I'll revert that once we've come to an answer to ^ that question.

> +  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.

Yeah that's exactly the case I was trying to protect against. I've fixed the
case you highlighted was wrong though. I added a helper method to stop the
duplication too.

> 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.

Oops, fixed!

(In reply to comment #2)
> (In reply to comment #1)
> > Do you really want _get_title()? Does that give a meaningful description? Would
> > the value of the FORM_TYPE field be better?
> 
> You noticed that it's never useful in the next branch... I still think
> FORM_TYPE would be useful.

http://okayface.com/okay-face.jpg

> Could any other code that uses gabble_presence_pick_resource_by_caps() be
> updated to use gabble_connection_pick_best_resource_for_caps() to delete
> duplicated code?

I don't think so. Most other places use the GabblePresence for something else,
and if they didn't then we'd be making log messages less specific...

> Ugh really? Use GHashTableIter. Or if you insist, g_hash_table_find, which lets
> you terminate the iteration when you find what you're after.

Hm, yes, I wonder why I didn't do that before. I remember being annoyed there
was no way to signal "okay don't go any further" in foreach. Anyway, fixed,
thanks.

I threw some patches on top of my moar-caps branch.

-- 
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.