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@xxxxxxxxxx>
Thanks,
> _______________________________________________
> 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