Hi <div class="gmail_extra"> <div class="gmail_quote">On Wed, Dec 5, 2012 at 5:22 AM, Dunrong Huang <<a href="mailto:riegamaths@gmail.com" target="_blank">riegamaths@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This patch is aimed to handle various file xfer messages. How it works: 0) our main channel introduces a API spice_main_file_xfer(). </blockquote><div> In general we prefer more human name in API, such as spice_main_file_copy() Imho, it should follow gio async model, so a client can track progress of each calls. (look at g_file_copy_async) I think a client API such as this one would fit better: spice_main_file_copy_async (GFile**, SpiceFileCopyFlags .., GCancellable, GFileProgressCallback, gpointer, GAsyncReadyCallback, gpointer) </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 1) When user drags a file and drop to spice client, spice client will catch a signal "drag-data-received", then it should call spice_main_file_xfer() for transfering file to guest. 2) In main channel: when spice_main_file_xfer() get called with file list passed, the API will send a start message which includes file and other needed information for each file. Then it will create a new xfer task and insert task list for each file, and return to caller. </blockquote><div> We shouldn't create several tasks simultaneously for a single call, it should be serialized to minimize completion time of each file. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 3) According to the response message sent from guest, our main channel decides whether send more data, or cancel this xfer task. 4) When file transfer has finished, file xfer task will be removed from task list. Signed-off-by: Dunrong Huang <<a href="mailto:riegamaths@gmail.com">riegamaths@gmail.com</a>> --- gtk/channel-main.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++ gtk/channel-main.h | 1 + gtk/map-file | 1 + gtk/spice-glib-sym-file | 1 + 4 files changed, 205 insertions(+) diff --git a/gtk/channel-main.c b/gtk/channel-main.c index 6b9ba8d..b2253a6 100644 --- a/gtk/channel-main.c +++ b/gtk/channel-main.c @@ -18,6 +18,7 @@ #include <math.h> #include <spice/vd_agent.h> #include <common/rect.h> +#include <glib/gstdio.h> #include "glib-compat.h" #include "spice-client.h" @@ -51,6 +52,12 @@ typedef struct spice_migrate spice_migrate; +typedef struct SpiceFileXferTask { + uint32_t id; + SpiceChannel *channel; + GFileInputStream *file_stream; +} SpiceFileXferTask; + struct _SpiceMainChannelPrivate { enum SpiceMouseMode mouse_mode; bool agent_connected; @@ -79,6 +86,7 @@ struct _SpiceMainChannelPrivate { } display[MAX_DISPLAY]; gint timer_id; GQueue *agent_msg_queue; + GList *file_xfer_task_list; guint switch_host_delayed_id; guint migrate_delayed_id; @@ -1384,6 +1392,100 @@ static void main_handle_agent_disconnected(SpiceChannel *channel, SpiceMsgIn *in agent_stopped(SPICE_MAIN_CHANNEL(channel)); } +static gboolean send_data(gpointer opaque) +{ +#define FILE_XFER_CHUNK_SIZE (VD_AGENT_MAX_DATA_SIZE - sizeof(VDAgentMessage)) + SpiceFileXferTask *task = (SpiceFileXferTask *)opaque; + + SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(task->channel); + SpiceMainChannelPrivate *c = channel->priv; + gssize len; + GError *error = NULL; + VDAgentFileXferDataMessage *msg; + uint32_t msg_size; + + if (!g_queue_is_empty(c->agent_msg_queue)) { + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + return TRUE; </blockquote><div> You can't use Idle functions this way, you are going to busy-loop. You need to handle filling the queue differently. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + } + + msg = g_alloca(sizeof(VDAgentFileXferDataMessage) + FILE_XFER_CHUNK_SIZE); + + len = g_input_stream_read(G_INPUT_STREAM(task->file_stream), msg->data, + FILE_XFER_CHUNK_SIZE, NULL, &error); </blockquote><div> Use async API g_input_stream_read_async instead. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (len == -1) { + /* TODO: error handler, tell guest cancel xfer */ + CHANNEL_DEBUG(channel, "Read file error: %s", error->message); + g_clear_error(&error); + goto done; + } else if (len == 0) { + CHANNEL_DEBUG(channel, "Finished Reading file"); + goto done; + } + + msg_size = sizeof(VDAgentFileXferDataMessage) + len; + msg->id = task->id; + msg->size = len; + agent_msg_queue(channel, VD_AGENT_FILE_XFER_DATA, msg_size, msg); + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + + return TRUE; + +done: + c->file_xfer_task_list = g_list_remove(c->file_xfer_task_list, task); + g_object_unref(task->file_stream); + g_free(task); + return FALSE; +} + +static gint file_xfer_task_find(gconstpointer a, gconstpointer b) +{ + SpiceFileXferTask *task = (SpiceFileXferTask *)a; + uint32_t id = *(uint32_t *)b; + + if (task->id == id) { + return 0; + } + + return 1; +} + +static void file_xfer_send_data_msg(SpiceChannel *channel, uint32_t id) +{ + SpiceMainChannelPrivate *c = SPICE_MAIN_CHANNEL(channel)->priv; + GList *l; + + l = g_list_find_custom(c->file_xfer_task_list, &id, + file_xfer_task_find); + if (l) { </blockquote><div> Use g_return_if_fail(l != NULL); instead, it will log a warning in this case, and you don't need a conditionnal block. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + g_idle_add(send_data, l->data); + } +} + +/* coroutine context */ +static void file_xfer_handle_status(SpiceChannel *channel, + VDAgentFileXferStatusMessage *msg) +{ + SPICE_DEBUG("task %d received response %d", msg->id, msg->result); + + if (msg->result == VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA) { + file_xfer_send_data_msg(channel, msg->id); + } else { + /* Error, remove this task */ + SpiceMainChannelPrivate *c = SPICE_MAIN_CHANNEL(channel)->priv; + GList *l; + l = g_list_find_custom(c->file_xfer_task_list, &msg->id, + file_xfer_task_find); + if (l) { </blockquote><div> Use g_return_if_fail(l != NULL); instead, it will log a warning in this case, and you don't need a conditionnal block. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + SpiceFileXferTask *task = l->data; + SPICE_DEBUG("user removed task %d, result: %d", msg->id, msg->result); + c->file_xfer_task_list = g_list_remove(c->file_xfer_task_list, task); + g_object_unref(task->file_stream); + g_free(task); + } + } +} + /* coroutine context */ static void main_agent_handle_msg(SpiceChannel *channel, VDAgentMessage *msg, gpointer payload) @@ -1487,6 +1589,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(channel, (VDAgentFileXferStatusMessage *)payload); + break; default: g_warning("unhandled agent message type: %u (%s), size %u", msg->type, NAME(agent_msg_types, msg->type), msg->size); @@ -2246,3 +2351,100 @@ void spice_main_set_display_enabled(SpiceMainChannel *channel, int id, gboolean c->display[id].enabled = enabled; } } + +static GFileInputStream *file_xfer_file_stream_new(const gchar *file_path) +{ + GFile *f; + GFileInputStream *stream; + GError *error = NULL; + + f = g_file_new_for_path(file_path); + stream = g_file_read(f, NULL, &error); </blockquote><div> Please use g_file_read_async() </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (error) { + SPICE_DEBUG("create file stream for %s error: %s", + file_path, error->message); + g_error_free(error); + } + g_object_unref(f); + + return stream; +} + +/* Create a new file xfer start msg, msg_size is total size of this package */ +static VDAgentFileXferStartMessage *file_xfer_start_msg_new(gchar *file_path, int *msg_size) +{ + static uint32_t xfer_id; /* Used to identify msg id */ + GStatBuf st; + VDAgentFileXferStartMessage *msg; + gchar *basename; + GKeyFile *keyfile; + gchar *data; + gsize data_len; + + keyfile = g_key_file_new(); + + /* File name */ + basename = g_path_get_basename(file_path); + g_key_file_set_string(keyfile, "vdagent-file-xfer", "name", basename); + g_free(basename); + + /* File size */ + g_stat(file_path, &st); + g_key_file_set_uint64(keyfile, "vdagent-file-xfer", "size", st.st_size); </blockquote><div>g_stat is a blocking IO API. Please use an asynchronous version, g_file_input_stream_query_info_async () </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + data = g_key_file_to_data(keyfile, &data_len, NULL); </blockquote><div> You may want to check the GError there. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + g_key_file_free(keyfile); + + *msg_size = sizeof(VDAgentFileXferStartMessage) + data_len + 1; + msg = g_malloc0(*msg_size); + xfer_id = (xfer_id > 10000) ? 0 : xfer_id; </blockquote><div> 10000 sounds like a reachable amount for file copy. Why not use the whole uint32 value range? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + msg->id = ++xfer_id; + memcpy(msg->data, data, data_len + 1); + + g_free(data); + return msg; +} + +static void file_xfer_send_start_msg(gpointer data, gpointer user_data) +{ + gchar *file_path; + GFileInputStream *file_stream; + SpiceFileXferTask *xfer; + VDAgentFileXferStartMessage *msg; + int msg_size; + SpiceMainChannel *channel = user_data; + + file_path = g_filename_from_uri((gchar *)data, NULL, NULL); + g_return_if_fail(file_path); + + file_stream = file_xfer_file_stream_new(file_path); + if (file_stream == NULL) { + g_free(file_path); + return ; + } + + msg = file_xfer_start_msg_new(file_path, &msg_size); + g_free(file_path); + + /* Create a new xfer task and insert into task list */ + xfer = spice_malloc0(sizeof(SpiceFileXferTask)); + xfer->id = msg->id; + xfer->channel = (SpiceChannel *)channel; + xfer->file_stream = file_stream; + + CHANNEL_DEBUG(channel, "Insert a xfer task:%d to task list", xfer->id); + channel->priv->file_xfer_task_list = g_list_append( + channel->priv->file_xfer_task_list, xfer); + + agent_msg_queue(channel, VD_AGENT_FILE_XFER_START, msg_size, msg); + g_free(msg); + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); +} + +gboolean spice_main_file_xfer(SpiceMainChannel *channel, GList *files, GError *err) +{ </blockquote><div> Please check validity of arguments in public functions (see code doing g_return_val_if_fail G_IS_SPICE_MAIN_CHANNEL..) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + /* Send VD_AGENT_FILE_XFER_START message to guest for every file */ </blockquote><div> For the reasons given above, please serialize each request or create a single request. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + g_list_foreach(files, file_xfer_send_start_msg, channel); + g_list_free_full(files, g_free); </blockquote><div> It's an easier to use API if you don't take the reference on the arguments. Don't request caller to give you allocated strings etc. Arguments should be const if possible. I suggest it also uses a GFile** array instead of a a glist of strings. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + return TRUE; +} diff --git a/gtk/channel-main.h b/gtk/channel-main.h index 1a5ab54..31997f1 100644 --- a/gtk/channel-main.h +++ b/gtk/channel-main.h @@ -78,6 +78,7 @@ void spice_main_clipboard_selection_notify(SpiceMainChannel *channel, guint sele void spice_main_clipboard_selection_request(SpiceMainChannel *channel, guint selection, guint32 type); gboolean spice_main_agent_test_capability(SpiceMainChannel *channel, guint32 cap); +gboolean spice_main_file_xfer(SpiceMainChannel *channel, GList *files, GError *err); #ifndef SPICE_DISABLE_DEPRECATED SPICE_DEPRECATED_FOR(spice_main_clipboard_selection_grab) diff --git a/gtk/map-file b/gtk/map-file index ed2c07f..7827726 100644 --- a/gtk/map-file +++ b/gtk/map-file @@ -53,6 +53,7 @@ spice_inputs_motion; spice_inputs_position; spice_inputs_set_key_locks; spice_main_agent_test_capability; +spice_main_file_xfer; spice_main_channel_get_type; spice_main_clipboard_grab; spice_main_clipboard_notify; diff --git a/gtk/spice-glib-sym-file b/gtk/spice-glib-sym-file index 82955f0..4bf92c3 100644 --- a/gtk/spice-glib-sym-file +++ b/gtk/spice-glib-sym-file @@ -29,6 +29,7 @@ spice_inputs_motion spice_inputs_position spice_inputs_set_key_locks spice_main_agent_test_capability +spice_main_file_xfer spice_main_channel_get_type spice_main_clipboard_grab spice_main_clipboard_notify -- 1.8.0 _______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </blockquote></div> thanks again for your work! -- Marc-André Lureau </div>