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

[Jonathon Jongsma]

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

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

> +
>  static void
>  spice_file_transfer_task_get_property(GObject *object,
>                                        guint property_id,


Acked-by: Jonathon Jongsma <jjongsma at redhat.com>