[Spice-devel] [spice-gtk v3 05/16] file-xfer: inform agent of errors only when task finished

[Victor Toso]

Hi,

On Fri, Jun 03, 2016 at 10:17:05AM -0500, Jonathon Jongsma wrote:
> Related to my comments to the last patch, this change makes the
> SpiceFileTransferTask class less "self-contained". In other words, if
> the user does not connect to the 'finished' signal and send the
> appropriate agent XFER_STATUS message in that handler, the file
> transfer won't work properly.

I don't follow.

Although the user/application can attach to SpiceFileTransferTask'
signals, we definitely don't rely on it.

Channel-main connects to the 'finished' and once that is emitted we will
catch that on task_finished() which should inform the agent the error.
That removes the need of SpiceFileTransferTask to send error to the
agent.

The user/application can either know the error from 'finished' or from
the ending callback call which, at the time of this patch, still comes
from SpiceFileTransferTask which means that g_task_return_error will be
called when the stream is closed at file_xfer_close_cb();

In the end of the series I change that slightly as I don't think that we
want to call the user callback from spice_main_file_copy_async() per
SpiceFileTransferTask but still, the user/application should be able to
get the error without connecting to 'finished'

> I think it's probably more maintainable and less error-prone if the
> SpiceFileTransferTask has the responsibility for all of the logic for
> conducting the file transfer.

I don't really agree, for me it makes sense to have clear the tasks of
each object so we can deal the situations more easily. The
SpiceFileTransferTask does get the file-info, the data from file,
progressbar info, handles Cancellation; Channel-Main is in charge with
communication with the agent, flushing data, start/stop/reset
file-transfer.

>
> I see Pavel already acked this patch, so I'm curious to hear both of your
> thoughts about the above comments.
>
> Jonathon

Cheers,
  toso

>
>
> On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> > No need to inform of a problem under
> > spice_file_transfer_task_completed() as the task will be finalized and
> > we can send the error to the agent there.
> > 
> > This change is related to split SpiceFileTransferTask from
> > channel-main.
> > 
> > Acked-by: Pavel Grunt <pgrunt at redhat.com>
> > ---
> >  src/channel-main.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 0ed322e..72dcf1f 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -2968,16 +2968,6 @@ static void
> > spice_file_transfer_task_completed(SpiceFileTransferTask *self,
> >          self->error = error;
> >      }
> >  
> > -    if (self->error) {
> > -        VDAgentFileXferStatusMessage msg = {
> > -            .id = self->id,
> > -            .result = self->error->code == G_IO_ERROR_CANCELLED ?
> > -                    VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> > VD_AGENT_FILE_XFER_STATUS_ERROR,
> > -        };
> > -        agent_msg_queue_many(self->channel, VD_AGENT_FILE_XFER_STATUS,
> > -                             &msg, sizeof(msg), NULL);
> > -    }
> > -
> >      if (self->pending)
> >          return;
> >  
> > @@ -3100,6 +3090,15 @@ static void task_finished(SpiceFileTransferTask *task,
> >      SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> >      guint32 task_id = spice_file_transfer_task_get_id(task);
> >  
> > +    if (error) {
> > +        VDAgentFileXferStatusMessage msg;
> > +        msg.id = task_id;
> > +        msg.result = error->code == G_IO_ERROR_CANCELLED ?
> > +                VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> > VD_AGENT_FILE_XFER_STATUS_ERROR;
> > +        agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS,
> > +                             &msg, sizeof(msg), NULL);
> > +    }
> > +
> >      g_hash_table_remove(channel->priv->file_xfer_tasks,
> > GUINT_TO_POINTER(task_id));
> >  }
> >