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

[Marc-André Lureau <marcandre.lureau@xxxxxxxxx>]

Hi

On Wed, Dec 5, 2012 at 5:22 AM, Dunrong Huang <riegamaths@xxxxxxxxx> 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@xxxxxxxxx>
---
 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 = "" &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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

thanks again for your work!

--
Marc-André Lureau

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