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@xxxxxxxxxx>
> > ---
> > 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));
> > }
> >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel