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