[Spice-commits] 3 commits - src/channel-main.c
Jonathon Jongsma
jjongsma at kemper.freedesktop.org
Thu Oct 22 08:40:05 PDT 2015
src/channel-main.c | 50 +++++++++++++++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 17 deletions(-)
New commits:
commit 51e5400d48efe24526c368e95689d085aa5cc959
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date: Wed Oct 21 15:26:54 2015 -0500
Remove noisy debug statement
This isn't terribly useful for general debugging and makes the log
pretty noisy when transferring large files.
diff --git a/src/channel-main.c b/src/channel-main.c
index 2493ead..0eb40b9 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -972,7 +972,6 @@ static gboolean file_xfer_flush_finish(SpiceMainChannel *channel, GAsyncResult *
return FALSE;
}
- CHANNEL_DEBUG(channel, "flushed finished!");
return g_simple_async_result_get_op_res_gboolean(simple);
}
commit e0ecb6985d18903d54e822089de1888329614001
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date: Wed Oct 21 14:52:57 2015 -0500
Don't print error message on successful file transfer
In certain circumstances we were printing an error message even though
the file transfer had completed successfully. It didn't cause any
problems, but it pointed out an issue in the handling of outgoing agent
messages.
The described behavior was generally only encountered when there were multiple
files being transferred at once, and most often when one or two files were
significantly smaller than another file being transferred. For example, two
20kB files and another 3MB file. The failure mechanism is basically as follows:
For each file transfer, we read a chunk for the file and then we queue a series
of messages to the guest and wait for them to be flushed before continuing to
read a new chunk of the file. (Since the maximum agent message size is 2kB, each
64kB chunk of file being read can generate up to 32 messages at a time). The
file transfer task remains in "pending" state until the flush operation is
complete. Since all agent messages go into a single channel-wide queue, waiting
for the messages to be flushed meant waiting for *all* outgoing messages to be
flushed, including those that were queued after we called flush_async().
Since agent messages can only be sent if the client has enough agent tokens, it
can take a little while for all queued messages to be sent. This means that
even if a file transfer task has sent out all of its data to the guest, it can
be kept in "pending" state for some time if other file transfer tasks or other
agent messages are added to the queue. In this situation, The guest might
notice that it has received all of the data for a file transfer task and send a
XFER_STATUS_SUCCESS message while we're still in "pending" state.
This patch changes to code so that flush_async() only waits for the
currently-queued messages to be sent. This means that it is not affected by any
new messages that get added to the outgoing queue after we've called
flush_async(). This means that the task will exit "pending" state immediately
after sending out our last data packet.
Fixes: rhbz#1265562
diff --git a/src/channel-main.c b/src/channel-main.c
index a833078..2493ead 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -152,7 +152,7 @@ struct _SpiceMainChannelPrivate {
gint timer_id;
GQueue *agent_msg_queue;
GHashTable *file_xfer_tasks;
- GSList *flushing;
+ GHashTable *flushing;
guint switch_host_delayed_id;
guint migrate_delayed_id;
@@ -289,6 +289,8 @@ static void spice_main_channel_init(SpiceMainChannel *channel)
c->agent_msg_queue = g_queue_new();
c->file_xfer_tasks = g_hash_table_new_full(g_direct_hash, g_direct_equal,
NULL, g_object_unref);
+ c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
+ g_object_unref);
c->cancellable_volume_info = g_cancellable_new();
spice_main_channel_reset_capabilties(SPICE_CHANNEL(channel));
@@ -406,6 +408,7 @@ static void spice_main_channel_dispose(GObject *obj)
}
g_clear_pointer(&c->file_xfer_tasks, g_hash_table_unref);
+ g_clear_pointer (&c->flushing, g_hash_table_unref);
g_cancellable_cancel(c->cancellable_volume_info);
g_clear_object(&c->cancellable_volume_info);
@@ -917,20 +920,22 @@ static void agent_free_msg_queue(SpiceMainChannel *channel)
c->agent_msg_queue = NULL;
}
-/* Here, flushing algorithm is stolen from spice-channel.c */
-static void file_xfer_flushed(SpiceMainChannel *channel, gboolean success)
+static gboolean flush_foreach_remove(gpointer key G_GNUC_UNUSED,
+ gpointer value, gpointer user_data)
{
- SpiceMainChannelPrivate *c = channel->priv;
- GSList *l;
+ gboolean success = GPOINTER_TO_UINT(user_data);
+ GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(value);
- for (l = c->flushing; l != NULL; l = l->next) {
- GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(l->data);
- g_simple_async_result_set_op_res_gboolean(result, success);
- g_simple_async_result_complete_in_idle(result);
- }
+ g_simple_async_result_set_op_res_gboolean(result, success);
+ g_simple_async_result_complete_in_idle(result);
+ return TRUE;
+}
- g_slist_free_full(c->flushing, g_object_unref);
- c->flushing = NULL;
+static void file_xfer_flushed(SpiceMainChannel *channel, gboolean success)
+{
+ SpiceMainChannelPrivate *c = channel->priv;
+ g_hash_table_foreach_remove(c->flushing, flush_foreach_remove,
+ GUINT_TO_POINTER(success));
}
static void file_xfer_flush_async(SpiceMainChannel *channel, GCancellable *cancellable,
@@ -951,7 +956,8 @@ static void file_xfer_flush_async(SpiceMainChannel *channel, GCancellable *cance
return;
}
- c->flushing = g_slist_append(c->flushing, simple);
+ /* wait until the last message currently in the queue has been sent */
+ g_hash_table_insert(c->flushing, g_queue_peek_tail(c->agent_msg_queue), simple);
}
static gboolean file_xfer_flush_finish(SpiceMainChannel *channel, GAsyncResult *result,
@@ -978,11 +984,22 @@ static void agent_send_msg_queue(SpiceMainChannel *channel)
while (c->agent_tokens > 0 &&
!g_queue_is_empty(c->agent_msg_queue)) {
+ GSimpleAsyncResult *simple;
c->agent_tokens--;
out = g_queue_pop_head(c->agent_msg_queue);
spice_msg_out_send_internal(out);
+
+ simple = g_hash_table_lookup(c->flushing, out);
+ if (simple) {
+ /* if there's a flush task waiting for this message, finish it */
+ g_simple_async_result_set_op_res_gboolean(simple, TRUE);
+ g_simple_async_result_complete_in_idle(simple);
+ g_hash_table_remove(c->flushing, out);
+ }
}
- if (g_queue_is_empty(c->agent_msg_queue) && c->flushing != NULL) {
+ if (g_queue_is_empty(c->agent_msg_queue) &&
+ g_hash_table_size(c->flushing) != 0) {
+ g_warning("unexpected flush task in list, clearing");
file_xfer_flushed(channel, TRUE);
}
}
commit 0c7643cb7f5c5411daff5f6e5d8a823b48ac874a
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date: Wed Oct 21 14:30:43 2015 -0500
MainChannel: move task free from finalize to dispose
In order to avoid reference cycles, you're supposed to release
references in dispose, especially to those objects that can hold
references to yourself. This probably wasn't causing any leaks, since
the file transfer tasks generally are not alive when the main channel is
destroyed, but it's more proper.
diff --git a/src/channel-main.c b/src/channel-main.c
index 3a8c1dd..a833078 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -405,6 +405,8 @@ static void spice_main_channel_dispose(GObject *obj)
c->migrate_delayed_id = 0;
}
+ g_clear_pointer(&c->file_xfer_tasks, g_hash_table_unref);
+
g_cancellable_cancel(c->cancellable_volume_info);
g_clear_object(&c->cancellable_volume_info);
@@ -418,8 +420,6 @@ static void spice_main_channel_finalize(GObject *obj)
g_free(c->agent_msg_data);
agent_free_msg_queue(SPICE_MAIN_CHANNEL(obj));
- if (c->file_xfer_tasks)
- g_hash_table_unref(c->file_xfer_tasks);
if (G_OBJECT_CLASS(spice_main_channel_parent_class)->finalize)
G_OBJECT_CLASS(spice_main_channel_parent_class)->finalize(obj);
More information about the Spice-commits
mailing list