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

[Victor Toso]

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel