[Spice-devel] [PATCH spice-gtk V5 1/3] file-xfer: handling various transfer messages in main channel

[Dunrong Huang]

On Thu, Jan 10, 2013 at 9:21 AM, Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
> Hi
>
> On Sat, Jan 5, 2013 at 11:59 AM, Dunrong Huang <riegamaths at gmail.com> wrote:
>> V4 -> V5:
>>    * Imporve error report
>>    * Add spice_main_file_copy_finish() function.
>>    * Some code cleanup. (e.g. file_xfer_task_free())
>>    * Limit spice_main_file_copy() to a single file.
>
> This last version is pretty close to being mergeable. It seems spicy
> hangs after doing a file copy. And apparently it's partly my fault
> since you copied a bug :)
>
>      if (was_empty) {
>          g_simple_async_result_set_op_res_gboolean(simple, TRUE);
>          g_simple_async_result_complete_in_idle(simple);
> +        g_object_unref(simple);
>          return;
>      }
>
> Can you confirm the bug/fix and update your patch?
Because I tested this feature by using remote-viewer, I cant get this
error. But I can confirm this bug exists
in spicy, and this small change fixes the bug.

It will be fixed in V6.
>
>>  gtk/channel-main.c      | 511 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  gtk/channel-main.h      |  12 ++
>>  gtk/map-file            |   2 +
>>  gtk/spice-glib-sym-file |   2 +
>>  4 files changed, 527 insertions(+)
>>
>> diff --git a/gtk/channel-main.c b/gtk/channel-main.c
>> index 6b9ba8d..e23d67d 100644
>> --- a/gtk/channel-main.c
>> +++ b/gtk/channel-main.c


>> +static gboolean
>> +file_xfer_flush_finish(SpiceMainChannel *channel, GAsyncResult *result,
>> +                       GError **error)
>> +{
>> +    GSimpleAsyncResult *simple;
>> +
>> +    simple = (GSimpleAsyncResult *)result;
>> +
>> +    if (g_simple_async_result_propagate_error(simple, error)) {
>> +        return -1;
>
> return FALSE
Ok.
>

>> +static void file_read_cb(GObject *source_object,
>> +                         GAsyncResult *res,
>> +                         gpointer user_data);
>> +static void file_xfer_continue_read(SpiceFileXferTask *task);
>> +
>> +static gboolean report_progress(gpointer user_data)
>> +{
>
> Make the function void and take SpiceFileXferTask * as argument
Ok.
>
>> +    SpiceFileXferTask *task = user_data;
>> +
>> +    if (task->progress_callback) {
>> +        task->progress_callback(task->read_bytes, task->file_size,
>> +                                task->progress_callback_data);
>> +    }
>> +
>> +    return FALSE;
>> +}
>> +
>> +static void data_flushed_cb(GObject *source_object,
>> +                            GAsyncResult *res,
>> +                            gpointer user_data)
>> +{
>> +    SpiceFileXferTask *task = user_data;
>> +    SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
>> +    GError *error = NULL;
>> +
>> +    file_xfer_flush_finish(channel, res, &error);
>> +
>> +    if (error != NULL) {
>> +        g_warning("failed to flush xfer queue: %s", error->message);
>> +        g_clear_error(&error);
>
> Async should probably take error and complete in this case.
Ok.
>
>> +    }
>> +
>> +    /* Report progress */
>> +    report_progress(task);
>> +
>> +    /* Read more data */
>> +    file_xfer_continue_read(task);
>> +}
>> +
>> +static void
>> +file_xfer_queue(SpiceFileXferTask *task, int data_size)
>> +{
>> +    VDAgentFileXferDataMessage *msg;
>> +    SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(task->channel);
>> +
>> +    msg = g_alloca(sizeof(VDAgentFileXferDataMessage));
>> +    msg->id = task->id;
>> +    msg->size = data_size;
>> +    agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_DATA, msg,
>> +                         sizeof(VDAgentFileXferDataMessage), task->buffer,
>> +                         data_size, NULL);
>> +    spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
>> +}
>> +
>> +static void file_xfer_task_free(SpiceFileXferTask *task)
>> +{
>> +    SpiceMainChannelPrivate *c;
>> +
>> +    g_return_if_fail(task != NULL);
>> +
>> +    c = task->channel->priv;
>> +    c->file_xfer_task_list = g_list_remove(c->file_xfer_task_list,
>> +                                           task);
>> +
>> +    if (task->file) {
>> +        g_object_unref(task->file);
>> +    }
>> +    if (task->file_stream) {
>> +        g_object_unref(task->file_stream);
>> +    }
>
> small nit: you may want to use g_clear_object(&foo) which is a bit
> shorter and safer.
>
Good tips. :-).
>> +    g_free(task);
>> +}
>> +

-- 
Best Regards,

Dunrong Huang