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?
>
> >
> > - 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.
>
> >
> >
> > 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
