Created attachment 87122 [details] [review]
client-types: implement RequestClientTypes()

Comment on attachment 87122 [details] [review]
client-types: implement RequestClientTypes()

Review of attachment 87122 [details] [review]:
-----------------------------------------------------------------

::: src/conn-client-types.c
@@ +153,5 @@
> +      g_error_free (error);
> +      return;
> +    }
> +
> +  DEBUG ("GetClientTypes called on the following handle: %u", contact);

That isn't its name.

@@ +157,5 @@
> +  DEBUG ("GetClientTypes called on the following handle: %u", contact);
> +
> +  if (!get_client_types_from_handle (conn, contact, &types))
> +    {
> +      DEBUG ("  waiting for disco reply");

Do we not have API for "disco them and wait for a response" already?

If we can't see the presence of any of their resources, shouldn't we give up immediately, rather than timing out?

@@ +160,5 @@
> +    {
> +      DEBUG ("  waiting for disco reply");
> +
> +      g_hash_table_insert (conn->client_types_pending,
> +              GUINT_TO_POINTER (contact), context);

If two RequestClientTypes calls overlap, the first one will never get a reply and the resources will never be freed.

Similarly, if we never get a disco result for that contact, we'll never send a reply and the resources will never be freed.

If waiting for disco isn't feasible, I think implementing this as a copy of GetClientTypes(), and opening a bug saying "implementation of RequestClientTypes should wait", would be better than nothing.

(In reply to comment #2)
> Comment on attachment 87122 [details] [review] [review]
> client-types: implement RequestClientTypes()
> 
> Review of attachment 87122 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/conn-client-types.c
> @@ +153,5 @@
> > +      g_error_free (error);
> > +      return;
> > +    }
> > +
> > +  DEBUG ("GetClientTypes called on the following handle: %u", contact);
> 
> That isn't its name.

fixed.

> @@ +157,5 @@
> > +  DEBUG ("GetClientTypes called on the following handle: %u", contact);
> > +
> > +  if (!get_client_types_from_handle (conn, contact, &types))
> > +    {
> > +      DEBUG ("  waiting for disco reply");
> 
> Do we not have API for "disco them and wait for a response" already?

Yeah, I could use gabble_disco_request_with_timeout() but that will fire a new disco query. I don't think we have API to ask to be notified when the existing disco is completed.

> If we can't see the presence of any of their resources, shouldn't we give up
> immediately, rather than timing out?

get_client_types_from_handle() returns an empty array in this case.

> @@ +160,5 @@
> > +    {
> > +      DEBUG ("  waiting for disco reply");
> > +
> > +      g_hash_table_insert (conn->client_types_pending,
> > +              GUINT_TO_POINTER (contact), context);
> 
> If two RequestClientTypes calls overlap, the first one will never get a
> reply and the resources will never be freed.
> 
> Similarly, if we never get a disco result for that contact, we'll never send
> a reply and the resources will never be freed.

Good point.

(In reply to comment #3)
> If waiting for disco isn't feasible, I think implementing this as a copy of
> GetClientTypes(), and opening a bug saying "implementation of
> RequestClientTypes should wait", would be better than nothing.

Ok, I've done that for now as that's not a regression anyway.

Created attachment 87130 [details] [review]
new patch

It'll do for now. Thanks!

Merged to master.