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

[Pavel Grunt]

Hi Jonathon,

On Fri, 2016-06-03 at 10:17 -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
> 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 see Pavel already acked this patch, so I'm curious to hear both of your
> thoughts about the above comments.

It is about defining what the task should do. The changes of behaviour you
mentioned should be documented. In my opinion the main channel (the user) should
care about the status of the transfer task, so it should connect to the signal.

Maybe the steps are rather big and detaching from the main channel should be
done slowly (exporting some agent functions/symbols in a private header and
using them as you suggested in review for the patch 4).

Pavel

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