[Spice-devel] [PATCH spice-gtk 2/2] file-xfer: Inform client of errors on init of xfer

[Pavel Grunt]

Hi,

On Mon, 2017-07-03 at 15:28 +0200, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> With SpiceFileTransferTask we suggest that Spice clients watch
> (signal) SpiceMainChannel::new-file-transfer so it can follow the
... so they can..
> transfer's progress till the moment it is finished.
> 
> On (signal) SpiceFileTransferTask::finished, we will issue if any
> error has happened to the application.

imo this sentence is hard to read. What about:
 The signal SpiceFileTransferTask::finished informs if an error happens to the
application


> 
> This patch solves the problem of SpiceFileTransferTask::finished not
> being emitted when the agent is not connected nor when file-transfer
> is disabled in the host.
> 
> We should fist emit SpiceMainChannel::new-file-transfer followed up by
first

> SpiceFileTransferTask::finished, which is done by the function
> spice_file_transfer_task_completed().
> 
> As channel-main chains up the "finish" signal by removing the
> SpiceFileTransferTask from its GHashTable, we have to change from
> GHashTableIter to a simple GList of keys.
okay
> 
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1373830
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/channel-main.c | 50 ++++++++++++++++++++++----------------------------
>  1 file changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 1c77c7b..0140d5b 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -3103,9 +3103,9 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>                                  gpointer user_data)
>  {
>      SpiceMainChannelPrivate *c;
> -    GHashTableIter iter;
> -    gpointer key, value;
>      FileTransferOperation *xfer_op;
> +    GError *error = NULL;
> +    GList *it, *keys;
>  
>      g_return_if_fail(channel != NULL);
>      g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> @@ -3113,25 +3113,13 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>  
>      c = channel->priv;
>      if (!c->agent_connected) {
> -        g_task_report_new_error(channel,
> -                                callback,
> -                                user_data,
> -                                spice_main_file_copy_async,
> -                                SPICE_CLIENT_ERROR,
> -                                SPICE_CLIENT_ERROR_FAILED,
> -                                _("The agent is not connected"));
> -        return;
> -    }
> -
> -    if (test_agent_cap(channel, VD_AGENT_CAP_FILE_XFER_DISABLED)) {
> -        g_task_report_new_error(channel,
> -                                callback,
> -                                user_data,
> -                                spice_main_file_copy_async,
> -                                SPICE_CLIENT_ERROR,
> -                                SPICE_CLIENT_ERROR_FAILED,
> -                                _("The file transfer is disabled"));
> -        return;
> +        error = g_error_new(SPICE_CLIENT_ERROR,
> +                            SPICE_CLIENT_ERROR_FAILED,
> +                            _("The agent is not connected"));
> +    } else if (test_agent_cap(channel, VD_AGENT_CAP_FILE_XFER_DISABLED)) {
> +        error = g_error_new(SPICE_CLIENT_ERROR,
> +                            SPICE_CLIENT_ERROR_FAILED,
> +                            _("The file transfer is disabled"));
>      }
>  
>      xfer_op = g_new0(FileTransferOperation, 1);
> @@ -3144,23 +3132,29 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
>                                                                 flags,
>                                                                 cancellable);
>      xfer_op->stats.num_files = g_hash_table_size(xfer_op->xfer_task);
> -    g_hash_table_iter_init(&iter, xfer_op->xfer_task);
> -    while (g_hash_table_iter_next(&iter, &key, &value)) {
> +    keys = g_hash_table_get_keys(xfer_op->xfer_task);
> +    for (it = keys; it != NULL; it = it->next) {
>          guint32 task_id;
> -        SpiceFileTransferTask *xfer_task = value;
> +        SpiceFileTransferTask *xfer_task = g_hash_table_lookup(xfer_op-
> >xfer_task, it->data);
>  
>          task_id = spice_file_transfer_task_get_id(xfer_task);
>  
>          SPICE_DEBUG("Insert a xfer task:%u to task list", task_id);
>  
> -        g_hash_table_insert(c->file_xfer_tasks, key, xfer_op);
> +        g_hash_table_insert(c->file_xfer_tasks, it->data, xfer_op);
>          g_signal_connect(xfer_task, "finished",
> G_CALLBACK(file_transfer_operation_task_finished), NULL);
>          g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> xfer_task);
>  
> -        spice_file_transfer_task_init_task_async(xfer_task,
> -                                                 file_xfer_init_task_async_cb
> ,
> -                                                 xfer_op);
> +        if (error == NULL) {
> +            spice_file_transfer_task_init_task_async(xfer_task,
> +                                                     file_xfer_init_task_asyn
> c_cb,
> +                                                     xfer_op);
> +        } else {
> +            spice_file_transfer_task_completed(xfer_task,
> g_error_copy(error));
> +        }
>      }
> +    g_list_free(keys);
> +    g_clear_pointer(&error, g_error_free);

g_clear_error

Thanks,
Pavel

>  }
>  
>  /**