It'd be cool if we could write plugins which did more with caps, like adding data forms to the GabbleCapsChannelManager.represent_client vfunc and stuff like that. Here's a branch which exposes more.
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.
(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. 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? + +typedef struct { + GabblePresence *presence; + TpHandle handle; +} GetJidData; + +static void +find_presence (gpointer key, + gpointer value, + gpointer user_data) +{ + GetJidData *data = user_data; + + if (data->presence == value) + data->handle = GPOINTER_TO_UINT (key); +} + +TpHandle +gabble_presence_cache_get_handle (GabblePresenceCache *cache, + GabblePresence *presence) +{ + GetJidData data = { presence, 0 }; + + g_hash_table_foreach (cache->priv->presence, find_presence, &data); + + return data.handle; +} 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.
(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.
(In reply to comment #3) > (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. I would go with: warn and drop the bad/duplicated forms, ideally (in the duplicated case) saying which other channel manager provided the same form in the warning. Even more ideally, maybe we could prefer to use forms from channel managers within Gabble… but broadly speaking I think just warning and dropping them will work out okay.
(In reply to comment #4) > I would go with: warn and drop the bad/duplicated forms, ideally (in the > duplicated case) saying which other channel manager provided the same form in > the warning. Even more ideally, maybe we could prefer to use forms from channel > managers within Gabble… but broadly speaking I think just warning and dropping > them will work out okay. Okay I've done this. So, http://www.youtube.com/watch?v=lrMD_z_FnNk#t=6s
Weeeellllllll. The problem with this is, if two different clients cause a CapsChannelManager to produce two forms with the same FORM_TYPE, we'll get a warning. (Also if two channel managers produce a Now, I don't know of many XEPs which actually use data forms in caps. (http://xmpp.org/extensions/xep-0232.html is the only one that comes to mind.) In the Ytstenut case, we have an implicit assumption that there's only one handler for each service, so we won't get duplicated FORM_TYPEs. So in practice this is not a problem for now. Possibly in future the channel manager API could be changed to allow them to have some way to merge duplicated forms—for duplicated feature URIs the merging operation is just union, but for forms it's harder. +static const gchar * +get_form_type (WockyDataForm *form) +{ + WockyDataFormField *field; + + field = g_hash_table_lookup (form->fields, + "FORM_TYPE"); + g_assert (field != NULL); + + return field->raw_value_contents[0]; +} Ugh this should be in wocky_data_form. (I know I know RESOLVED LATER.) So I think this branch is fine for now, but noting the above caveat in the code would be good.
Merged, thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.