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 <[email protected]> > > > --- > > > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
