[Spice-devel] [spice-gtk v4 07/24] main: to let SpiceFileTransferTask handle errors

[Victor Toso]

Hi,

On Fri, Jun 24, 2016 at 02:17:33PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-06-23 at 19:37 +0200, Victor Toso wrote:
> > * spice_file_transfer_task_read_async() will call the callback with
> >   error in case it is called on pending state
> 
> OK, this maintains behavior, since it just moves the error reporting down a
> level to the called function.
> 
> > 
> > * on VD_AGENT_FILE_XFER_STATUS_SUCCESS it should not be possible to be
> >   on pending state as spice_file_transfer_task_read_async() would
> >   return immediately in case all file is read
> 
> On the other hand, this appears to change behavior. If the guest sends an early
> STATUS_SUCCESS to the client for some reason (buggy agent thinks it received all
> data even though the client is still sending data), we will no longer report an
> error. I'm not sure that's a good idea.

Agreed!

Seems that we can move this down to spice_file_transfer_task_completed()
as this function should be called to complete a task, it seems wrong to
complete a task without error but in a pending state.

<snip>
-    if (self->pending)
+    if (self->pending) {
+        /* Complete but pending is okay only if error is set */
+        if (self->error == NULL) {
+            self->error = g_error_new(SPICE_CLIENT_ERROR,
+                                      SPICE_CLIENT_ERROR_FAILED,
+                                      "Cannot complete task in pending
state");
+        }
         return;
+    }
</snip>

This will require some tests, I guess (to avoid regressions)
  toso

> 
> 
> > ---
> >  src/channel-main.c | 8 --------
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 9787613..4b728fe 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -1993,11 +1993,6 @@ static void
> > spice_file_transfer_task_handle_status(SpiceFileTransferTask *task,
> >  
> >      switch (msg->result) {
> >      case VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA:
> > -        if (task->pending) {
> > -            error = g_error_new(SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > -                           "transfer received CAN_SEND_DATA in pending
> > state");
> > -            break;
> > -        }
> >          spice_file_transfer_task_read_async(task, file_xfer_read_async_cb,
> > NULL);
> >          return;
> >      case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
> > @@ -2009,9 +2004,6 @@ static void
> > spice_file_transfer_task_handle_status(SpiceFileTransferTask *task,
> >                              "some errors occurred in the spice agent");
> >          break;
> >      case VD_AGENT_FILE_XFER_STATUS_SUCCESS:
> > -        if (task->pending)
> > -            error = g_error_new(SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > -                                "transfer received success in pending
> > state");
> >          break;
> >      default:
> >          g_warn_if_reached();
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel