Re: [spice-gtk v4 05/24] file-xfer: introduce functions to init task async

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Jun 23, 2016 at 03:39:58PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-06-23 at 19:37 +0200, Victor Toso wrote:
> > Introduced functions (private):
> > * void       spice_file_transfer_task_init_task_async()
> > * GFileInfo *spice_file_transfer_task_init_task_finish()
> > 
> > The init process of SpiceFileTransferTask does initialize its
> > GFileInputStream (for reading the file) and also its GFileInfo
> > (necessary to protocol in order to start the file-transfer with agent)
> > 
> > Due the logic changed involved, some functions were renamed to better
> > match its place and purpose:
> > 
> > * file_xfer_info_async_cb -> file_xfer_init_task_async_cb
> >   It is channel-main's callback for each _init_task_async()
> > 
> > * file_xfer_read_async_cb -> spice_file_transfer_task_read_file_cb
> > * file_xfer_info_async_cb -> spice_file_transfer_task_query_info_cb
> >   Both should be private to SpiceFileTransferTask now.
> > 
> > As the _init_task_async() uses GTask, some error handling was moved to
> > channel-main's after _init_task_finish() is called.
> > 
> > This change is related to split SpiceFileTransferTask from
> > channel-main.
> > ---
> >  src/channel-main.c | 181 +++++++++++++++++++++++++++++++++++++---------------
> > -
> >  1 file changed, 126 insertions(+), 55 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 1411fb3..ceca7e3 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -65,6 +65,12 @@ static GHashTable
> > *spice_file_transfer_task_create_tasks(GFile **files,
> >                                                           gpointer
> > progress_callback_data,
> >                                                           GAsyncReadyCallback
> > callback,
> >                                                           gpointer user_data);
> > +static void spice_file_transfer_task_init_task_async(SpiceFileTransferTask
> > *self,
> > +                                                     GAsyncReadyCallback
> > callback,
> > +                                                     gpointer userdata);
> > +static GFileInfo
> > *spice_file_transfer_task_init_task_finish(SpiceFileTransferTask *xfer_task,
> > +                                                            GAsyncResult
> > *result,
> > +                                                            GError **error);
> >  
> >  /**
> >   * SECTION:file-transfer-task
> > @@ -3025,35 +3031,32 @@ signal:
> >  }
> >  
> >  
> > -static void file_xfer_info_async_cb(GObject *obj, GAsyncResult *res, gpointer
> > data)
> > +static void file_xfer_init_task_async_cb(GObject *obj, GAsyncResult *res,
> > gpointer data)
> >  {
> >      GFileInfo *info;
> > -    GFile *file = G_FILE(obj);
> > -    GError *error = NULL;
> > -    GKeyFile *keyfile = NULL;
> > -    gchar *basename = NULL;
> > +    SpiceFileTransferTask *xfer_task;
> > +    SpiceMainChannel *channel;
> > +    gchar *string, *basename;
> > +    GKeyFile *keyfile;
> >      VDAgentFileXferStartMessage msg;
> > -    gsize /*msg_size*/ data_len;
> > -    gchar *string;
> > -    SpiceFileTransferTask *self = SPICE_FILE_TRANSFER_TASK(data);
> > +    guint64 file_size;
> > +    gsize data_len;
> > +    GError *error = NULL;
> >  
> > -    self->pending = FALSE;
> > -    info = g_file_query_info_finish(file, res, &error);
> > -    if (error || self->error)
> > +    xfer_task = SPICE_FILE_TRANSFER_TASK(obj);
> > +
> > +    info = spice_file_transfer_task_init_task_finish(xfer_task, res, &error);
> > +    if (info == NULL)
> >          goto failed;
> >  
> > -    self->file_info = info;
> > -    self->file_size =
> > -        g_file_info_get_attribute_uint64(info,
> > G_FILE_ATTRIBUTE_STANDARD_SIZE);
> > -    g_object_notify(G_OBJECT(self), "progress");
> > -    keyfile = g_key_file_new();
> > +    channel = spice_file_transfer_task_get_channel(xfer_task);
> > +    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);
> >  
> > -    /* File name */
> > -    basename = g_file_get_basename(file);
> > +    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);
> > -    /* 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 */
> > @@ -3063,39 +3066,16 @@ static void file_xfer_info_async_cb(GObject *obj,
> > GAsyncResult *res, gpointer da
> >          goto failed;
> >  
> >      /* Create file-xfer start message */
> > -    msg.id = self->id;
> > -    agent_msg_queue_many(self->channel, VD_AGENT_FILE_XFER_START,
> > +    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(self->channel), FALSE);
> > +    spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE);
> 
> [NOTE: read comments below first]
> 
> If you follow my suggestions below, you'll need to unref 'info' here.
> 
> >      return;
> >  
> >  failed:
> > -    spice_file_transfer_task_completed(self, error);
> > -}
> > -
> > -static void file_xfer_read_async_cb(GObject *obj, GAsyncResult *res, gpointer
> > data)
> > -{
> > -    GFile *file = G_FILE(obj);
> > -    SpiceFileTransferTask *self = SPICE_FILE_TRANSFER_TASK(data);
> > -    GError *error = NULL;
> > -
> > -    self->pending = FALSE;
> > -    self->file_stream = g_file_read_finish(file, res, &error);
> > -    if (error || self->error) {
> > -        spice_file_transfer_task_completed(self, error);
> > -        return;
> > -    }
> > -
> > -    g_file_query_info_async(self->file,
> > -                            G_FILE_ATTRIBUTE_STANDARD_SIZE,
> > -                            G_FILE_QUERY_INFO_NONE,
> > -                            G_PRIORITY_DEFAULT,
> > -                            self->cancellable,
> > -                            file_xfer_info_async_cb,
> > -                            self);
> > -    self->pending = TRUE;
> > +    spice_file_transfer_task_completed(xfer_task, error);
> >  }
> >  
> >  static SpiceFileTransferTask *spice_file_transfer_task_new(SpiceMainChannel
> > *channel,
> > @@ -3184,12 +3164,10 @@ void spice_main_file_copy_async(SpiceMainChannel
> > *channel,
> >      g_hash_table_iter_init(&iter, xfer_ht);
> >      while (g_hash_table_iter_next(&iter, &key, &value)) {
> >          guint32 task_id;
> > -        GFile *file;
> >          SpiceFileTransferTask *xfer_task;
> >  
> >          xfer_task = value;
> >          task_id = spice_file_transfer_task_get_id(xfer_task);
> > -        g_object_get(xfer_task, "file", &file, NULL);
> >  
> >          SPICE_DEBUG("Insert a xfer task:%u to task list", task_id);
> >  
> > @@ -3197,12 +3175,9 @@ void spice_main_file_copy_async(SpiceMainChannel
> > *channel,
> >          g_signal_connect(xfer_task, "finished", G_CALLBACK(task_finished),
> > channel);
> >          g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> > xfer_task);
> >  
> > -        g_file_read_async(file,
> > -                          G_PRIORITY_DEFAULT,
> > -                          cancellable,
> > -                          file_xfer_read_async_cb,
> > -                          xfer_task);
> > -        xfer_task->pending = TRUE;
> > +        spice_file_transfer_task_init_task_async(xfer_task,
> > +                                                 file_xfer_init_task_async_cb
> > ,
> > +                                                 NULL);
> >      }
> >      g_hash_table_unref(xfer_ht);
> >  }
> > @@ -3296,6 +3271,102 @@ static GHashTable
> > *spice_file_transfer_task_create_tasks(GFile **files,
> >      return xfer_ht;
> >  }
> >  
> > +static void spice_file_transfer_task_query_info_cb(GObject *obj,
> > +                                                   GAsyncResult *res,
> > +                                                   gpointer user_data)
> > +{
> > +    SpiceFileTransferTask *self;
> > +    GFileInfo *info;
> > +    GTask *task;
> > +    GError *error = NULL;
> > +
> > +    task = G_TASK(user_data);
> > +    self = g_task_get_source_object(task);
> > +
> > +    g_return_if_fail(self->pending == TRUE);
> > +    self->pending = FALSE;
> > +
> > +    info = g_file_query_info_finish(G_FILE(obj), res, &error);
> > +    if (error || self->error) {
> > +        error = (error == NULL) ? self->error : error;
> > +        g_task_return_error(task, error);
> > +        return;
> > +    }
> > +
> > +    self->file_info = info;
> 
> don't need to save this: see below.
> 
> > +    self->file_size =
> > +        g_file_info_get_attribute_uint64(info,
> > G_FILE_ATTRIBUTE_STANDARD_SIZE);
> > +
> > +    /* SpiceFileTransferTask's init is done, handshake for file-trasfer will
> > +     * start soon. First "progress" can be emitted ~ 0% */
> > +    g_object_notify(G_OBJECT(self), "progress");
> > +
> > +    g_task_return_boolean(task, TRUE);
>
> Instead of returning a boolean here, I think it would be better to do:
>
> g_task_return_pointer(task, info);
>
> Then see suggested changes below to _init_task_finish()

Right!

>
> > +}
> > +
> > +static void spice_file_transfer_task_read_file_cb(GObject *obj,
> > +                                                  GAsyncResult *res,
> > +                                                  gpointer user_data)
> > +{
> > +    SpiceFileTransferTask *self;
> > +    GTask *task;
> > +    GError *error = NULL;
> > +
> > +    task = G_TASK(user_data);
> > +    self = g_task_get_source_object(task);
> > +
> > +    g_return_if_fail(self->pending == TRUE);
> > +
> > +    self->file_stream = g_file_read_finish(G_FILE(obj), res, &error);
> > +    if (error || self->error) {
> > +        self->pending = FALSE;
> > +        error = (error == NULL) ? self->error : error;
> > +        g_task_return_error(task, error);
> > +        return;
> > +    }
> > +
> > +    g_file_query_info_async(self->file,
> > +                            "standard::*",
> > +                            G_FILE_QUERY_INFO_NONE,
> > +                            G_PRIORITY_DEFAULT,
> > +                            self->cancellable,
> > +                            spice_file_transfer_task_query_info_cb,
> > +                            task);
> > +}
> > +
> > +static void spice_file_transfer_task_init_task_async(SpiceFileTransferTask
> > *self,
> > +                                                     GAsyncReadyCallback
> > callback,
> > +                                                     gpointer userdata)
> > +{
> > +    GTask *task;
> > +
> > +    g_return_if_fail(self != NULL);
> > +    g_return_if_fail(self->pending == FALSE);
> > +
> > +    task = g_task_new(self, self->cancellable, callback, userdata);
> > +
> > +    self->pending = TRUE;
> > +    g_file_read_async(self->file,
> > +                      G_PRIORITY_DEFAULT,
> > +                      self->cancellable,
> > +                      spice_file_transfer_task_read_file_cb,
> > +                      task);
> > +}
> > +
> > +static GFileInfo
> > *spice_file_transfer_task_init_task_finish(SpiceFileTransferTask *self,
> > +                                                            GAsyncResult
> > *result,
> > +                                                            GError **error)
> > +{
> > +    GTask *task = G_TASK(result);
> > +
> > +    g_return_val_if_fail(self != NULL, NULL);
> > +
> > +    if (g_task_propagate_boolean(task, error))
> > +        return self->file_info;
> > +
> > +    return NULL;
>
> This function can basically just be:
>
> return g_task_propagate_pointer(task, error);

Indeed :-)

I'll send the changed patch in-reply-too here and I'll keep the rebased
version in the gitlab [0] repo for now.

Many thanks,
  toso

[0] (move-xfer-last) https://gitlab.com/victortoso/spice-gtk

>
> > +}
> > +
> >  static void
> >  spice_file_transfer_task_get_property(GObject *object,
> >                                        guint property_id,
> 
> 
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]