[Spice-devel] [spice-gtk v4 18/24] file-xfer: use helper function for debug xfer-task

[Victor Toso]

Hi,

On Mon, Jun 27, 2016 at 03:53:03PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-06-23 at 19:37 +0200, Victor Toso wrote:
> > ---
> >  src/channel-main.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 63f0b9b..2c9a6fb 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -76,6 +76,7 @@ static gssize
> > spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
> >                                                     GError **error);
> >  static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> > *self);
> >  static guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask
> > *self);
> > +static void spice_file_transfer_task_debug_info(SpiceFileTransferTask *self);
> >  
> >  /**
> >   * SECTION:file-transfer-task
> > @@ -1893,18 +1894,8 @@ static void file_xfer_data_flushed_cb(GObject
> > *source_object,
> >  
> >      file_transfer_operation_send_progress(self);
> >  
> > -    if (spice_util_get_debug()) {
> > -        const GTimeSpan interval = 20 * G_TIME_SPAN_SECOND;
> > -        gint64 now = g_get_monotonic_time();
> > -
> > -        if (interval < now - self->last_update) {
> > -            gchar *basename = g_file_get_basename(self->file);
> > -            self->last_update = now;
> > -            SPICE_DEBUG("transferred %.2f%% of the file %s",
> > -                        100.0 * self->read_bytes / self->file_size,
> > basename);
> > -            g_free(basename);
> > -        }
> > -    }
> > +    if (spice_util_get_debug())
> > +        spice_file_transfer_task_debug_info(self);
> >  
> >      /* Read more data */
> >      spice_file_transfer_task_read_async(self, file_xfer_read_async_cb, NULL);
> > @@ -3532,6 +3523,20 @@ static gssize
> > spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
> >      return nbytes;
> >  }
> >  
> > +static void spice_file_transfer_task_debug_info(SpiceFileTransferTask *self)
> > +{
> > +    const GTimeSpan interval = 20 * G_TIME_SPAN_SECOND;
> > +    gint64 now = g_get_monotonic_time();
> > +
> > +    if (interval < now - self->last_update) {
> > +        gchar *basename = g_file_get_basename(self->file);
> > +        self->last_update = now;
> > +        SPICE_DEBUG("transferred %.2f%% of the file %s",
> > +                    100.0 * self->read_bytes / self->file_size, basename);
> > +        g_free(basename);
> > +    }
> > +}
>
> It seems a bit odd to embed the time-based reporting policy inside of
> this function. I understand that you're extracting this so that it can
> later be moved to a separate file and the task will remain
> encapsulated. So maybe that's fine.
> Or there are a couple alternatives we could consider:
>
> - print a debug progress statement after reading the file instead of
> after flushing it -- then we could keep the entire thing inside of the
> spice-file-transfer-task.c and wouldn't need to expose a _debug_info()
> function to be called from channel-main.c

I think this one is better as we already keep the 'last_update' info in
SpiceFileTransferTask. I'll change it.

>
> - print a debug progress statement at the FileTransferOperation level rather
> than at the Task level. Then we could move keep it all contained
> within channel- main.c
>
> Again, I may be being too nitpicky here, so feel free to ignore if you
> want.

One function less to be exported (which was there only for debug) so I
think this makes the code better :)

>
> > +
> >  static void
> >  spice_file_transfer_task_get_property(GObject *object,
> >                                        guint property_id,
>
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

Thanks,

> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel