On Tue, Jun 07, 2016 at 10:22:12AM -0500, Jonathon Jongsma wrote:
> On Sun, 2016-06-05 at 10:20 +0200, Victor Toso wrote:
> > 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 :)
>
> OK, this seems reasonable. I think a proper callback works better than a
> signal.
> Since you only really need to notify the caller, a generic signal seems like
> overkill and makes the API a bit less foolproof. Perhaps the name should be
> _query_info_async() to follow glib naming conventions?
Sure :)
>
>
> >
> > >
> > > - 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 :)
>
> sure, we can discuss it later.
Cheers,
toso
>
> >
> > >
> > >
> > > Reviewed-by: Jonathon Jongsma <jjong...@redhat.com>
> > 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_ST
> > > > RING
> > > > 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__BOX
> > > > ED,
> > > > + 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
