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

[Jonathon Jongsma]

On Sun, 2016-06-05 at 10:38 +0200, Victor Toso wrote:
> 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'

Hmm, re-reading my comment, I find it hard to understand as well. Sorry I wasn't
more clear. In essence, I was working from the assumption that the
SpiceFileTransferTask should be fully in charge of the file transfer. In other
words, when you call _start(), it should handle all of the file transfer logic
and send all of the messages and indicate the status back to the MainChannel.
>From that point of view, when the SpiceFileTransferTask emits "finished", the
transfer should actually be finished. But it is not actually finished yet,
because we haven't yet sent the VD_AGENT_FILE_XFER_STATUS message in the case
where there has been an error. 

But since my assumption above was wrong, the above comment doesn't make much
sense. Even so, I think that a "finished" signal is perhaps not the best way to
handle this. see below.

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

OK, so I'll accept your division of responsibilities here. But I still think
that the design could be improved a bit. I have some vague ideas that I'm still
mulling over and will try to present them in a future email after I determine
whether they're workable or not.

> 
> > 
> > 
> > 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));
> > >  }
> > >