[Spice-devel] [spice-gtk v5 00/23] move/improve SpiceFileTransferTask

[Victor Toso]

Hi,

On Wed, Jul 06, 2016 at 11:22:54AM -0500, Jonathon Jongsma wrote:
> On Tue, 2016-07-05 at 16:12 +0200, Victor Toso wrote:
> > Hi,
> > 
> > And I forgot to send the link to the repo upstream, sorry.
> > https://gitlab.com/victortoso/spice-gtk.git
> > 
> > On Tue, Jul 05, 2016 at 03:07:07PM +0200, Victor Toso wrote:
> > > 
> > > Hi,
> > > 
> > > Most of patches were acked but sending them once more before pushing.
> > > Changes from v4 are:
> > > 
> > > (Numbers based on previous series)
> > > * Commit log improvements in patches: 02, 06, 07, 08, 09, 10, 12, 14,
> > >   18, 21
> > > * Comments/typos fixes in patches: 03, 21
> > > * FIXMEs to address other concerns were included in patches: 03
> > > * Dropped patch 04 as we don't need to save GFileInfo anymore
> > > * Logic changes regarding dealing with two different GErrors so we can
> > >   easily say the one with high priority.
> > > * 0006: notify "progress" when read-async is called so the progress info
> > >   is always related to the data sent instead of data read
> > > * 0007: Moved error-checking from spice_file_transfer_task_handle_status
> > >   in channel-main.c to spice_file_transfer_task_completed() currently at
> > >   channel-main.c but will be moved to
> > >   spice_file_transfer_task_completed.c;The error is related to complete
> > >   task while file-transfer is in pending state
> > > * 0008: renamed function from file_xfer_handle_status() to
> > >   main_agent_handle_xfer_status
> > > * 0010: Removed SpiceFileTransferTask argument from
> > >   file_xfer_queue_msg_to_agent
> > > * 0012: "file-xfer: a FileTransferOperation per transfer call"
> > >  - removing type reference from variables name ('*_ht' as GHashTable);
> > >  - renamed: file_transfer_operation_reset_all ->
> > >    spice_main_channel_reset_all_xfer_operations
> > >  - renamed: file_transfer_operation_find_task_by_id ->
> > >    spice_main_channel_find_xfer_task_by_task_id
> > >  - renamed: file_transfer_operation_end to file_transfer_operation_free
> > >  - included a warning in case trying to reset a xfer-task that does not
> > >    exist anymore
> > >  - newer gcc complains about possible null pointer dereference.. fixed.
> > > * 0014: Fix progress data info as suggested by Jonathon: On
> > >   error/cancel, we decrease the transfer_size by the amount of data not
> > >   yet sent from the failed task.
> > > * 0015: Set proper domain/code to G_IO_ERROR/_CANCELLED when all tasks
> > >   were cancelled
> > > * 0018: Changed the debug info related to amount of data transferred to
> > >   be the amount of data read and moved it to the read file callback
> > > * 0020: Fixed alignment on src/Makefile.am
> > > * 0021: Removed type reference from HashTable variable; reworked the small
> > >   summary regarding how a file transfer works
> > > * 0022: Added test to cancel *after* task_init is called
> > > * 0023: Added test to cancel *after* read_async is called
> > > 
> > > Cheers,
> > >   toso
> > > 
> > > Victor Toso (23):
> > >   file-xfer: task-id to start with 1
> > >   file-xfer: get functions for SpiceFileTransferTask
> > >   file-xfer: introduce _create_tasks()
> > >   file-xfer: introduce functions to init task async
> > >   file-xfer: introduce functions to read file async
> > >   main: to let SpiceFileTransferTask handle errors
> > >   main: let channel-main handle file-xfer messages
> > >   main: do not check for xfer-task errors
> > >   file-xfer: improve helper function to queue agent message
> > >   file-xfer: create helper function to send progress
> > >   file-xfer: a FileTransferOperation per transfer call
> > >   file-xfer: inform agent of errors only when task finished
> > >   file-xfer: call progress_callback per FileTransferOperation
> > >   file-xfer: call user callback once per operation
> > >   file-xfer: do not send EOF to the agent
> > >   file-xfer: Keep namespace of private function
> > >   file-xfer: move debug to SpiceFileTransferTask
> > >   main: use xfer_task instead of task or self
> > >   file-xfer: move to spice-file-transfer-task.c
> > >   tests: file-transfer include simple tests
> > >   tests: file-transfer cancel on task init
> > >   tests: file-transfer cancel on read async
> > >   tests: file-transfer agent cancel
> > > 
> > >  src/Makefile.am                     |   2 +
> > >  src/channel-main.c                  | 926 +++++++++++--------------------
> > > -----
> > >  src/spice-file-transfer-task-priv.h |  59 +++
> > >  src/spice-file-transfer-task.c      | 737 ++++++++++++++++++++++++++++
> > >  tests/Makefile.am                   |   2 +
> > >  tests/file-transfer.c               | 430 +++++++++++++++++
> > >  6 files changed, 1507 insertions(+), 649 deletions(-)
> > >  create mode 100644 src/spice-file-transfer-task-priv.h
> > >  create mode 100644 src/spice-file-transfer-task.c
> > >  create mode 100644 tests/file-transfer.c
> > >
>
> Thanks a lot. Looks great. There were a couple very minor comments on
> a few of the patches, but you can consider all of the rest to be
> ACKed.
>
> Jonathon

Many thanks for the reviews, really.

I removed the 'do not send EOF to the agent' patch to avoid regression
from https://bugzilla.redhat.com/show_bug.cgi?id=1135099;
I fixed the typos and included all the comments suggestions.

Pushed upstream 2fbdb68afa09afe .. 95c570274f2e041

Cheers,
  toso