[Spice-devel] [PATCH spice-gtk V3 1/3] file-xfer: handling various transfer messages in main channel
Marc-André Lureau
marcandre.lureau at gmail.com
Fri Dec 7 03:45:04 PST 2012
Hi
On Wed, Dec 5, 2012 at 5:22 AM, Dunrong Huang <riegamaths at gmail.com> wrote:
> This patch is aimed to handle various file xfer messages.
>
> How it works:
> 0) our main channel introduces a API spice_main_file_xfer().
>
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)
> 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.
>
We shouldn't create several tasks simultaneously for a single call, it
should be serialized to minimize completion time of each file.
>
> 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 <riegamaths at gmail.com>
> ---
> 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;
>
You can't use Idle functions this way, you are going to busy-loop. You need
to handle filling the queue differently.
> + }
> +
> + 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);
>
Use async API g_input_stream_read_async instead.
> + 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) {
>
Use g_return_if_fail(l != NULL); instead, it will log a warning in this
case, and you don't need a conditionnal block.
+ 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) {
>
Use g_return_if_fail(l != NULL); instead, it will log a warning in this
case, and you don't need a conditionnal block.
> + 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);
>
Please use g_file_read_async()
> + 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);
>
> g_stat is a blocking IO API. Please use an asynchronous version,
g_file_input_stream_query_info_async ()
> + data = g_key_file_to_data(keyfile, &data_len, NULL);
>
You may want to check the GError there.
> + 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;
>
10000 sounds like a reachable amount for file copy. Why not use the whole
uint32 value range?
> + 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)
> +{
>
Please check validity of arguments in public functions (see code doing
g_return_val_if_fail G_IS_SPICE_MAIN_CHANNEL..)
> + /* Send VD_AGENT_FILE_XFER_START message to guest for every file */
>
For the reasons given above, please serialize each request or create a
single request.
> + g_list_foreach(files, file_xfer_send_start_msg, channel);
> + g_list_free_full(files, g_free);
>
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.
+ 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
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
thanks again for your work!
--
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20121207/112040d7/attachment-0001.html>
More information about the Spice-devel
mailing list