Hi, Sorry for taking some time to reply :) On Tue, Jun 07, 2016 at 12:17:40PM -0500, Jonathon Jongsma wrote: > On Tue, 2016-06-07 at 11:14 -0500, Jonathon Jongsma wrote: > > 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. > > OK, so here's my attempt to flesh out my ideas for the design. > > the _start() function could be changed to something like: > > spice_file_transfer_task_query_info_async(SpiceFileTransferTask*, > GAsyncReadyCallback, ...) > spice_file_transfer_task_query_info_finish() > Yes, this will be much better. > One thing that seems a bit out-of-place to me is the "flush_callback" in > SpiceFileTransferTask. with the current design, the SpiceFileTransferTask > doesn't really need to know anything about the concept of "flushing". All it > needs to do is provide a chunk of data to the main channel to send. Then it > should just wait for the main channel to request the next chunk. The main > channel is the only one that should need to know anything about flushing. > > So perhaps we could remove the SpiceFileTransferTaskFlushCb callback and simply > replace it with something like the following API (based on > g_file_input_stream_read_async()): > > spice_file_transfer_task_read_next_chunk_async(SpiceFileTransferTask*, void* > buffer, gsize count, GAsyncReadyCallback, ...) > gssize spice_file_transfer_task_read_next_chunk_finish() > > In other words, we don't pass a flush_callback to the task when we create it; > this callback is instead passed as an argument to _read_next_chunk_async(). And > _read_next_chunk_async() is basically a replacement for > spice_file_transfer_task_flush_done() Your proposal looks cleaner and better to me. I plan to change it to follow it. > > [I thought about simply exposing the GFileInputStream to the MainChannel to do > those read operations (rather than providing the wrapper functions mentioned > above), but I think that would leak too much logic back into MainChannel, which > we're trying to avoid.] > > I don't know if those ideas help at all. Probably function names could be > improved, etc. And maybe attempting to implement something like that would > expose some problems with the design. Interested in your thoughts. > > Jonathon As you said, I'll try to follow your proposal and see if we can find issues later on. Once again, many thanks toso > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel