On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> In order to avoid sending the agent message on
> file_xfer_info_async_cb, we can provide the "file-info" signal to
> SpiceFileTransferTask.
>
> In order to this signal be significant to applications, we request all
> standard attributes to g_file_query_info_async.
>
I'd like to see some explanation for why this signal is useful, or is an
improvement from the previous behavior. For instance, *why* do we want to avoid
sending the agent message on file_xfer_info_async_cb()? I assume that it's
because after we split this into a different file, we will no longer have
access to agent_msg_queue_many(), but I'd like to have it in the commit log.
That said, I have to admit that this change feels a bit strange to me. For
example, when SpiceFileTransferTask is split out to its own self-contained
object, I would expect that if I called spice_file_transfer_task_start_task(),
it would start transferring the file. However, after this change, the only
thing it does is open the file stream and query the file info and then emit the
"file-info" signal. The MainChannel is required to handle the file-info signal
and start sending agent messages. But there's nothing about the function name
spice_file_transfer_task_start() that makes you think that this is how to use
this function. I understand that this is internal API, so it doesn't
necessarily need to be as polished as external API would be, but I wonder if it
could be improved.
I can think of a couple of possible options:
- rename spice_file_transfer_task_start_task() to something that makes it a
little bit more obvious what to expect. Maybe
spice_file_transfer_task_prepare(), or spice_file_transfer_task_query_info(),
or ???
- expose the agent_msg_queue() function (as
main_channel_queue_agent_message()?) in a (private?) header so that
SpiceFileTransferTask can initiate the agent communication itself
Thoughts?
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
> This change is related to split SpiceFileTransferTask from
> channel-main.
> ---
> src/channel-main.c | 96 ++++++++++++++++++++++++++++++++++++++---------------
> -
> 1 file changed, 68 insertions(+), 28 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 7843aeb..0ed322e 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -136,6 +136,7 @@ enum {
>
> enum {
> SIGNAL_FINISHED,
> + SIGNAL_FILE_INFO,
> LAST_TASK_SIGNAL
> };
>
> @@ -3001,11 +3002,6 @@ static void file_xfer_info_async_cb(GObject *obj,
> GAsyncResult *res, gpointer da
> 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;
> SpiceFileTransferTask *self = SPICE_FILE_TRANSFER_TASK(data);
>
> self->pending = FALSE;
> @@ -3015,33 +3011,14 @@ static void file_xfer_info_async_cb(GObject *obj,
> GAsyncResult *res, gpointer da
>
> self->file_size =
> g_file_info_get_attribute_uint64(info,
> G_FILE_ATTRIBUTE_STANDARD_SIZE);
> + g_signal_emit(self, task_signals[SIGNAL_FILE_INFO], 0, info);
> g_object_notify(G_OBJECT(self), "progress");
> - keyfile = g_key_file_new();
> + g_clear_object(&info);
>
> - /* File name */
> - basename = g_file_get_basename(file);
> - 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", self-
> >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.id = self->id;
> - agent_msg_queue_many(self->channel, VD_AGENT_FILE_XFER_START,
> - &msg, sizeof(msg),
> - string, data_len + 1, NULL);
> - g_free(string);
> - spice_channel_wakeup(SPICE_CHANNEL(self->channel), FALSE);
> return;
>
> failed:
> + g_clear_object(&info);
> spice_file_transfer_task_completed(self, error);
> }
>
> @@ -3059,7 +3036,7 @@ static void file_xfer_read_async_cb(GObject *obj,
> GAsyncResult *res, gpointer da
> }
>
> g_file_query_info_async(self->file,
> - G_FILE_ATTRIBUTE_STANDARD_SIZE,
> + "standard::*",
> G_FILE_QUERY_INFO_NONE,
> G_PRIORITY_DEFAULT,
> self->cancellable,
> @@ -3071,6 +3048,50 @@ static void file_xfer_read_async_cb(GObject *obj,
> GAsyncResult *res, gpointer da
> static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> *channel,
> GFile *file,
> GCancellable
> *cancellable);
> +static void file_xfer_on_file_info(SpiceFileTransferTask *xfer_task,
> + GFileInfo *info,
> + gpointer data)
> +{
> + SpiceMainChannel *channel;
> + GKeyFile *keyfile;
> + VDAgentFileXferStartMessage msg;
> + gchar *string, *basename;
> + guint64 file_size;
> + gsize data_len;
> + GError *error = NULL;
> +
> + g_return_if_fail(info != NULL);
> +
> + channel = SPICE_MAIN_CHANNEL(data);
> +
> + basename = g_file_info_get_attribute_as_string(info,
> G_FILE_ATTRIBUTE_STANDARD_NAME);
> + file_size = g_file_info_get_attribute_uint64(info,
> G_FILE_ATTRIBUTE_STANDARD_SIZE);
> +
> + keyfile = g_key_file_new();
> + g_key_file_set_string(keyfile, "vdagent-file-xfer", "name", basename);
> + g_key_file_set_uint64(keyfile, "vdagent-file-xfer", "size", file_size);
> +
> + g_free(basename);
> +
> + /* 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.id = spice_file_transfer_task_get_id(xfer_task);
> + agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_START,
> + &msg, sizeof(msg),
> + string, data_len + 1, NULL);
> + g_free(string);
> + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
> + return;
> +
> +failed:
> + spice_file_transfer_task_completed(xfer_task, error);
> +}
>
> static void task_finished(SpiceFileTransferTask *task,
> GError *error,
> @@ -3157,6 +3178,7 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
> guint32 task_id = spice_file_transfer_task_get_id(task);
>
> g_hash_table_insert(c->file_xfer_tasks, GUINT_TO_POINTER(task_id),
> task);
> + g_signal_connect(task, "file-info",
> G_CALLBACK(file_xfer_on_file_info), channel);
> g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> channel);
> g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> task);
> spice_file_transfer_task_start_task(task);
> @@ -3483,6 +3505,24 @@
> spice_file_transfer_task_class_init(SpiceFileTransferTaskClass *klass)
> G_PARAM_STATIC_STRING
> S));
>
> /**
> + * SpiceFileTransferTask::file-info
> + * @task: the file transfer task that emitted the signal
> + * @file_info: (transfer none): A GFileInfo object to retrieve file
> + * information. Only keys from standard namespace are supported.
> + *
> + * The #SpiceFileTransferTask::file-info signal is emitted just before
> the file
> + * transfer effectively starts.
> + *
> + * Since: 0.32
> + **/
> + task_signals[SIGNAL_FILE_INFO] = g_signal_new("file-info",
> SPICE_TYPE_FILE_TRANSFER_TASK,
> + G_SIGNAL_RUN_FIRST,
> + 0, NULL, NULL,
> + g_cclosure_marshal_VOID__BOXED,
> + G_TYPE_NONE, 1,
> + G_TYPE_FILE_INFO);
> +
> + /**
> * SpiceFileTransferTask::finished:
> * @task: the file transfer task that emitted the signal
> * @error: (transfer none): the error state of the transfer. Will be
> %NULL
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel