Bug 28015

I started implementing this but I'm wondering then the close_channels() operation should be completed:

a) When Claim() returns
b) When all the Close() calls returned

If we go for b), should we succeed only if *all* the Close calls succeeded?


I'm tempted to go for a) as we can't do much if Close() fails anyway.

(In reply to comment #1)
> I started implementing this but I'm wondering then the close_channels()
> operation should be completed:
> 
> a) When Claim() returns
> b) When all the Close() calls returned
> 
> If we go for b), should we succeed only if *all* the Close calls succeeded?
> 
> 
> I'm tempted to go for a) as we can't do much if Close() fails anyway.

Yeah, I agree. Maybe debug some stuff if Close() fails but… I don't see much benefit in propagating that up to the app, what's the app going to do?

Created attachment 47117 [details] [review]
factor out prepare_core_and_claim()

Created attachment 47118 [details] [review]
Add tp_channel_dispatch_operation_close_channels_async() (#28015)

I only implemented the close_channels version here but this can already be reviewed.

What should be the semantic of the 2 others calls? Should they return as soon as Destroy/RemoveMembers has been called on all channels?

Review of attachment 47117 [details] [review]:

Looks functionally fine but I have some refcounting and other nitpicks.

::: telepathy-glib/channel-dispatch-operation.c
@@ +1446,3 @@
   if (!tp_channel_dispatch_operation_claim_finish (self, result, &error))
     {
+      DEBUG ("Failed to Calim %s: %s",

Calim is my best friend's name.

@@ +1452,3 @@
+      g_simple_async_result_complete (ctx->result);
+      /* Remove the ref we got from the caller */
+      g_object_unref (ctx->result);

This is not very clear to me, really: you're having to do “more” freeing of the ctx on some but not all code paths, just so that the callback has to do its own unreffing of the result object?

Also this failure path is duplicated. I don't mind that much though. I do mind 'goto out' when there's exactly one line of code between the goto and the label and you could easily add an else block.

Review of attachment 47118 [details] [review]:

::: telepathy-glib/channel-dispatch-operation.c
@@ +1606,3 @@
+      TpChannel *channel = g_ptr_array_index (self->priv->channels, i);
+
+      g_error_free (error);

If the callback were to use its user_data, it would be unsafe because 'self' might have died before the callback got called.

But it doesn't, so you should pass NULL.

Review of attachment 47118 [details] [review]:

::: telepathy-glib/channel-dispatch-operation.c
@@ +1606,3 @@
+      TpChannel *channel = g_ptr_array_index (self->priv->channels, i);
+
+      g_error_free (error);

If the callback were to use its user_data, it would be unsafe because 'self' might have died before the callback got called.

But it doesn't, so you should pass NULL.

(In reply to comment #6)
> Review of attachment 47117 [details] [review]:
> 
> Looks functionally fine but I have some refcounting and other nitpicks.
> 
> ::: telepathy-glib/channel-dispatch-operation.c
> @@ +1446,3 @@
>    if (!tp_channel_dispatch_operation_claim_finish (self, result, &error))
>      {
> +      DEBUG ("Failed to Calim %s: %s",
> 
> Calim is my best friend's name.

fixed (squashed).

> @@ +1452,3 @@
> +      g_simple_async_result_complete (ctx->result);
> +      /* Remove the ref we got from the caller */
> +      g_object_unref (ctx->result);
> 
> This is not very clear to me, really: you're having to do “more” freeing of the
> ctx on some but not all code paths, just so that the callback has to do its own
> unreffing of the result object?

As said in prepare_core_and_claim's documentation, it takes the ref on @result
and pass it back to @callback is everything is fine. But if an error occurs,
@callback won't be called, the operation will be completed (with an error) and
so we have to drop this ref.

I made that clearer in the code.

> Also this failure path is duplicated. I don't mind that much though. I do mind
> 'goto out' when there's exactly one line of code between the goto and the label
> and you could easily add an else block.

I factored out prepare_core_and_claim_ctx_failed() removing the code
duplication.

(In reply to comment #7)
> Review of attachment 47118 [details] [review]:
> 
> ::: telepathy-glib/channel-dispatch-operation.c
> @@ +1606,3 @@
> +      TpChannel *channel = g_ptr_array_index (self->priv->channels, i);
> +
> +      g_error_free (error);
> 
> If the callback were to use its user_data, it would be unsafe because 'self'
> might have died before the callback got called.
> 
> But it doesn't, so you should pass NULL.

Sorry I don't understand what you mean.

I merged tp_channel_dispatch_operation_close_channels_async; will be in 0.15.1.

I keep the bug opened as we still miss the destroy and remove members case.

(In reply to comment #5)
> What should be the semantic of the 2 others calls? Should they return as soon
> as Destroy/RemoveMembers has been called on all channels?

Probably. In both cases I think they should try to Close() the channel if the method we really wanted to use doesn't work. But I don't see a great deal of point notifying the application if the channels can't be closed for some reason.

http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=cdo-28015 updated with the 2 other versions.

Created attachment 47310 [details] [review]
add tp_channel_dispatch_operation_leave_channels_async (#28015)

Created attachment 47311 [details] [review]
add tp_channel_destroy_async()

Created attachment 47312 [details] [review]
add tp_channel_dispatch_operation_destroy_channels_async() (fdo #28015)

Review of attachment 47311 [details] [review]:

::: telepathy-glib/channel.c
@@ +2402,3 @@
+        }
+
+  if (error != NULL)

I think there's way too much debugging output in this function. And a goto! I'd just DEBUG() if Destroy() fails, move the “Close(); return;” into get_invalidated() == NULL {}, and then you can lose the goto.

Review of attachment 47312 [details] [review]:

Looks fine.

Review of attachment 47310 [details] [review]:

looks fine.

Merged to master; will be in 0.15.2