Re: [PATCH spice-gtk V4 1/3] file-xfer: handling various transfer messages in main channel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, thanks for your review.

On Tue, Jan 1, 2013 at 1:51 AM, Marc-André Lureau
<marcandre.lureau@xxxxxxxxx> wrote:
>
> Hi!
>
> On Fri, Dec 28, 2012 at 2:33 PM, Dunrong Huang <riegamaths@xxxxxxxxx> wrote:
>>
>> Agent channel is a flow-control channel. That means before
>> we send agent data to server, we must obtain tokens distributed
>> from spice server, if we do not do that, spice server will get error,
>> or at least, the data will be discarded.
>>
>> Other type of agent data will be cached to agent_msg_queue if there
>> are no more tokens. But for file-xfer data, if we cache too much of
>> those data, our memory will be exhausted pretty quickly if file is
>> too big.
>>
>> We also should make other agent data(clipboard, mouse, ...) get
>> through when file-xfer data are sending.
>>
>> So, for the reason of above, we can not fill file-xfer data to agent queue
>> too quickly, we must consider the tokens, and other messages.
>>
>> Marc-André suggested me to call spice_channel_flush_async() and wait the
>> queued data to be sent, but the API does not consider the available
>> tokens, so I use a new algorithm/API(file_xfer_flush_async) based on
>> spice_channel_flush_async() to send file-xfer data.
>>
>
> Right
>


>> +
>> +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);
>
>
> Even if the function currently doesn't report error, you should treat error
> case at least with a g_warning().
>
Yes

>> +
>> +/* main context */
>> +static void file_read_cb(GObject *source_object,
>> +                         GAsyncResult *res,
>> +                         gpointer user_data)
>> +{
>> +    SpiceFileXferTask *task = user_data;
>> +    SpiceMainChannel *channel = task->channel;
>> +    gssize count;
>> +    GError *error = NULL;
>> +
>> +    count = g_input_stream_read_finish(G_INPUT_STREAM(task->file_stream),
>> +                                       res, &error);
>> +    if (count > 0) {
>> +        task->read_bytes += count;
>> +        file_xfer_queue(task, count);
>> +        file_xfer_flush_async(channel, task->cancellable,
>> +                              data_flushed_cb, task);
>> +    } else {
>> +        g_input_stream_close_async(G_INPUT_STREAM(task->file_stream),
>> +                                   G_PRIORITY_DEFAULT,
>> +                                   task->cancellable,
>> +                                   file_close_cb,
>> +                                   task);
>
>
> You should report error to caller (probably with
> g_simple_async_report_gerror_in_idle)
>
> There will be some issues with the fact that there are multiple outstanding
> async in the background. The common pattern is to _always_ complete one
> async() call  with one result (succesful or error). There shouldn't be
> "lost" async, or you may "block" some client execution path.
>
Will be fixed in next version.

>> +
>> +/* coroutine context */
>> +static void file_xfer_handle_status(SpiceMainChannel *channel,
>> +                                    VDAgentFileXferStatusMessage *msg)
>> +{
>> +    SPICE_DEBUG("task %d received response %d", msg->id, msg->result);
>
>
>  You could lookup the task only once here.
>
>> +
>> +    if (msg->result == VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA) {
>> +        file_xfer_send_data_msg(channel, msg->id);
>
>
> and can call g_input_stream_read_async() directly here, or perhaps move the
> read_async() in a seperate function task_continue_read() which will be
> called also from data_flushed_cb().
>
Will be fixed in next version.
>>
>> +    } else {
>
>
> Please check precisely the other msg->result values, and do a
> g_warn_if_reached() for unknown values.
>
Ok.
>> +        /* Error, remove this task */
>> +        SpiceMainChannelPrivate *c = channel->priv;
>> +        GList *l;
>> +        SpiceFileXferTask *task;
>> +
>> +        l = g_list_find_custom(c->file_xfer_task_list, &msg->id,
>> +                               file_xfer_task_find);
>> +        g_return_if_fail(l != NULL);
>> +
>> +        task = l->data;
>> +        SPICE_DEBUG("user removed task %d, result: %d", msg->id,
>> +                    msg->result);
>
>
> You should report error to caller (probably with
> g_simple_async_report_gerror_in_idle)
>
Ok.
>
>
>> +        c->file_xfer_task_list = g_list_remove(c->file_xfer_task_list,
>> +                                               task);
>> +        g_object_unref(task->file);
>> +        g_object_unref(task->file_stream);
>> +        g_free(task);
>
>
> Those last 4 lines could probably be in a seperate function
> file_xfer_task_free ()
>
Agree.
>> +    }
>> +}
>> +
>>  /* coroutine context */
>>  static void main_agent_handle_msg(SpiceChannel *channel,
>>                                    VDAgentMessage *msg, gpointer payload)
>> @@ -1487,6 +1765,9 @@ static void main_agent_handle_msg(SpiceChannel
>> *channel,
>>                      reply->error == VD_AGENT_SUCCESS ? "success" :
>> "error");
>>          break;
>>      }
>> +    case VD_AGENT_FILE_XFER_STATUS:
>> +        file_xfer_handle_status(SPICE_MAIN_CHANNEL(channel), payload);
>> +        break;
>>      default:
>>          g_warning("unhandled agent message type: %u (%s), size %u",
>>                    msg->type, NAME(agent_msg_types, msg->type),
>> msg->size);
>> @@ -1563,6 +1844,7 @@ static void main_handle_agent_token(SpiceChannel
>> *channel, SpiceMsgIn *in)
>>      SpiceMainChannelPrivate *c = SPICE_MAIN_CHANNEL(channel)->priv;
>>
>>      c->agent_tokens += tokens->num_tokens;
>> +
>>      agent_send_msg_queue(SPICE_MAIN_CHANNEL(channel));
>>  }
>>
>> @@ -2246,3 +2528,197 @@ void
>> spice_main_set_display_enabled(SpiceMainChannel *channel, int id, gboolean
>>          c->display[id].enabled = enabled;
>>      }
>>  }
>> +
>> +static void
>> +file_info_async_cb(GObject *obj, GAsyncResult *res, gpointer data)
>> +{
>> +    GFileInfo *info;
>> +    GFile *file = G_FILE(obj);
>> +    GError *error = NULL;
>> +    GKeyFile *keyfile = NULL;
>> +    gchar *basename = NULL;
>> +    VDAgentFileXferStartMessage *msg;
>> +    gsize msg_size, data_len;
>> +    gchar *string;
>> +    SpiceFileXferTask *task = (SpiceFileXferTask *)data;
>> +    SpiceMainChannelPrivate *c = task->channel->priv;
>> +
>> +    info = g_file_query_info_finish(file, res, &error);
>> +    if (error) {
>> +        SPICE_DEBUG("couldn't get size of file %s: %s",
>> +                    g_file_get_path(file),
>> +                    error->message);
>> +        goto failed;
>> +    }
>> +    task->file_size = g_file_info_get_attribute_uint64(info,
>> +                                        G_FILE_ATTRIBUTE_STANDARD_SIZE);
>> +
>> +    keyfile = g_key_file_new();
>> +    if (keyfile == NULL) {
>> +        SPICE_DEBUG("failed to create key file: %s", error->message);
>> +        goto failed;
>> +    }
>> +
>> +    /* File name */
>> +    basename = g_file_get_basename(file);
>> +    if (basename == NULL) {
>> +        SPICE_DEBUG("failed to get file basename: %s", error->message);
>> +        goto failed;
>> +    }
>> +    g_key_file_set_string(keyfile, "vdagent-file-xfer", "name",
>> basename);
>> +    g_free(basename);
>> +
>> +    /* File size */
>> +    g_key_file_set_uint64(keyfile, "vdagent-file-xfer", "size",
>> +                          task->file_size);
>> +
>> +    /* Save keyfile content to memory. TODO: more file attributions
>> +       need to be sent to guest */
>> +    string = g_key_file_to_data(keyfile, &data_len, &error);
>> +    g_key_file_free(keyfile);
>> +    if (error) {
>> +        goto failed;
>> +    }
>> +
>> +    /* Create file-xfer start message */
>> +    msg_size = sizeof(VDAgentFileXferStartMessage) + data_len + 1;
>> +    msg = g_malloc0(msg_size);
>
>
> This allocation and copy seems unnecessary, can you use "string" directly?
>
Ok, since msg->data is a zero-length array, we can do it by using
agent_msg_queue_many(). will be fixed next version.
>> +    msg->id = task->id;
>> +    memcpy(msg->data, string, data_len + 1);
>> +    g_free(string);
>> +
>> +    CHANNEL_DEBUG(task->channel, "Insert a xfer task:%d to task list",
>> +                  task->id);
>> +    c->file_xfer_task_list = g_list_append(c->file_xfer_task_list, task);
>> +
>> +    agent_msg_queue(task->channel, VD_AGENT_FILE_XFER_START, msg_size,
>> msg);
>> +    g_free(msg);
>> +    spice_channel_wakeup(SPICE_CHANNEL(task->channel), FALSE);
>> +    return ;
>> +
>> +failed:
>> +    g_clear_error(&error);
>> +    g_object_unref(task->file);
>> +    g_object_unref(task->file_stream);
>> +    g_free(task);
>> +}
>> +
>> +static void
>> +read_async_cb(GObject *obj, GAsyncResult *res, gpointer data)
>> +{
>> +    GFile *file = G_FILE(obj);
>> +    SpiceFileXferTask *task = (SpiceFileXferTask *)data;
>> +    GError *error = NULL;
>> +
>> +    task->file_stream = g_file_read_finish(file, res, &error);
>> +
>> +    if (task->file_stream) {
>> +        g_file_query_info_async(task->file,
>> +                                G_FILE_ATTRIBUTE_STANDARD_SIZE,
>> +                                G_FILE_QUERY_INFO_NONE,
>> +                                G_PRIORITY_DEFAULT,
>> +                                task->cancellable,
>> +                                file_info_async_cb,
>> +                                task);
>> +    } else {
>> +        SPICE_DEBUG("create file stream for %s error: %s",
>> +                    g_file_get_path(file), error->message);
>> +        g_clear_error(&error);
>> +        g_object_unref(task->file);
>> +        g_free(task);
>
>
> You should report error to caller (probably with
> g_simple_async_report_gerror_in_idle)
>
Ok.

>> +void spice_main_file_copy_async(SpiceMainChannel *channel,
>> +                                GFile **sources,
>> +                                GFileCopyFlags flags,
>> +                                GCancellable *cancellable,
>> +                                GFileProgressCallback progress_callback,
>> +                                gpointer progress_callback_data,
>> +                                GAsyncReadyCallback callback,
>> +                                gpointer user_data)
>> +{
>> +    int i = 0;
>> +    static uint32_t xfer_group_id;
>> +
>> +    g_return_if_fail(channel != NULL);
>> +    g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
>> +    g_return_if_fail(sources != NULL);
>> +
>> +    xfer_group_id++;
>> +    xfer_group_id = (xfer_group_id > UINT32_MAX) ? 0 : xfer_group_id;
>> +    while (sources[i]) {
>> +        /* All tasks created from below function have same group id */
>
>
> I am worried by the server side handling of sharing the same group id for
> several requests. But I am okay with this communication pattern that can be
> later improved if needed.
>

Hi, Marc-André

spice_main_file_copy_async() was designed to copy multi-files at a
time, which I think is a little unreasonable:

1) If something error happens when opening or reading a file. should
we report error immediately(using callback), or wait for all tasks to
complete?

2) We have to set group_id to identify tasks created at the same
time(e.g. for report_progress, report_finish, ...)

3) For reporting progress, we have to calculate all tasks' remain
bytes(see report_progress()), this is a bit weird. It should be done
by caller of this API(just as nautilus does).

4) After a task is finished, we have to check whether other tasks with
same group_id have been finished(see report_finish()). I think it's a
bit weird.

If spice_main_file_copy_async() only copys one file at a time, our
code will be more simple and clean. The caller of this API can
implement
coping multi-files, just as nautilus does.

So, our client API shoule be like this:
spice_main_file_copy_async(SpiceMainChannel *channel, GFile *,
GFileCopyFlags, GCancellable *, GFileProgressCallback , gpointer ,
GAsyncReadyCallback ,gpointer )

And we also need spice_main_file_copy_finish() to finish operation.

Can you agree with me?

-- 
Best Regards,

Dunrong Huang
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]