[Spice-devel] [spice-gtk v3 05/16] file-xfer: inform agent of errors only when task finished

[Jonathon Jongsma]

Related to my comments to the last patch, this change makes the
SpiceFileTransferTask class less "self-contained". In other words, if the user
does not connect to the 'finished' signal and send the appropriate agent
XFER_STATUS message in that handler, the file transfer won't work properly. I
think it's probably more maintainable and less error-prone if the
SpiceFileTransferTask has the responsibility for all of the logic for conducting
the file transfer.

I see Pavel already acked this patch, so I'm curious to hear both of your
thoughts about the above comments.

Jonathon


On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> No need to inform of a problem under
> spice_file_transfer_task_completed() as the task will be finalized and
> we can send the error to the agent there.
> 
> This change is related to split SpiceFileTransferTask from
> channel-main.
> 
> Acked-by: Pavel Grunt <pgrunt at redhat.com>
> ---
>  src/channel-main.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 0ed322e..72dcf1f 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -2968,16 +2968,6 @@ static void
> spice_file_transfer_task_completed(SpiceFileTransferTask *self,
>          self->error = error;
>      }
>  
> -    if (self->error) {
> -        VDAgentFileXferStatusMessage msg = {
> -            .id = self->id,
> -            .result = self->error->code == G_IO_ERROR_CANCELLED ?
> -                    VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> VD_AGENT_FILE_XFER_STATUS_ERROR,
> -        };
> -        agent_msg_queue_many(self->channel, VD_AGENT_FILE_XFER_STATUS,
> -                             &msg, sizeof(msg), NULL);
> -    }
> -
>      if (self->pending)
>          return;
>  
> @@ -3100,6 +3090,15 @@ static void task_finished(SpiceFileTransferTask *task,
>      SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
>      guint32 task_id = spice_file_transfer_task_get_id(task);
>  
> +    if (error) {
> +        VDAgentFileXferStatusMessage msg;
> +        msg.id = task_id;
> +        msg.result = error->code == G_IO_ERROR_CANCELLED ?
> +                VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> VD_AGENT_FILE_XFER_STATUS_ERROR;
> +        agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS,
> +                             &msg, sizeof(msg), NULL);
> +    }
> +
>      g_hash_table_remove(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task_id));
>  }
>