Hi,
On Thu, Jun 02, 2016 at 05:14:57PM -0500, Jonathon Jongsma wrote:
> 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.
I think I should have put much more info in the cover-letter, sorry!
So, my goals is to move SpiceFileTransferTask to its own file but also
trying to make it less aware on how to the protocol works. I did not
fully achieved that in the end of the series as you can see,
spice_file_transfer_task_handle_status() is still a helpful function to
deal with the status of the file-transfer based on agent messages.
But regarding flushing data (the callback) and this file-info signal,
that was the solution I got at the moment...
> 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.
You are right, it definitely can be improved. Taking a quick look on how
file-transfer actually works in the very beginning from protocol
point-of-view.
1-) (send) VD_AGENT_FILE_XFER_START needs file-name and file-size;
2-) (receive) VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA and we can
actually start the file-transfer
So:
>
> 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 ???
I would like to take this approach, renaming the _start_task to
spice_file_transfer_task_query_info which is exactly what we will do and
we can remove the file-info signal to a proper callback to this
function.
PS: I tried to convince myself that the signal could be useful to the
Application but I really can't see how :)
> - 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?
I have also the intent to move the agent code in channel-main to its own
file like channel-main-agent or something. I would agree then that we
should export a way to queue agent msgs outside channel-main but for
other channels if that makes sense. I would love to discuss that at some
point :)
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
Many thanks, I don't plan to push any other patch till we got this thing
right, I hope to send a v4 in the next few days.
Cheers,
toso
>
>
> >
> > 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