On Fri, 2016-11-11 at 23:53 +0100, Victor Toso wrote: > Hi, > > On Fri, Nov 11, 2016 at 04:05:21PM -0600, Jonathon Jongsma wrote: > > > > On Fri, 2016-11-11 at 14:27 +0100, Victor Toso wrote: > > > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > > > This fixes the situation when VDAgent receives 0 byte message > > > regarding a file-transfer operation that was terminated in the > > > previous message. > > > > > > This makes the VDAgent to send a STATUS_ERROR after > > > STATUS_SUCCESS to > > > client. > > > > > > Resolves: https://bugs.freedesktop.org/show_bug.cgi?id=97227 > > > > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > > --- > > > src/channel-main.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/src/channel-main.c b/src/channel-main.c > > > index 990a06a..72ca712 100644 > > > --- a/src/channel-main.c > > > +++ b/src/channel-main.c > > > @@ -1822,6 +1822,14 @@ static void > > > file_xfer_read_async_cb(GObject > > > *source_object, > > > return; > > > } > > > > > > + if (count == 0 && > > > spice_file_transfer_task_get_total_bytes(xfer_task) > 0) { > > > + /* If we have sent all payload to the agent, we should > > > not > > > send 0 bytes > > > + * as it will cause https://bugs.freedesktop.org/show_bu > > > g.cg > > > i?id=97227. > > > + * Only when file has 0 bytes of size is when we should > > > send > > > 0 bytes to > > > + * agent, see: https://bugzilla.redhat.com/show_bug.cgi? > > > id=1 > > > 135099 */ > > > + return; > > > + } > > > + > > > file_xfer_queue_msg_to_agent(channel, > > > spice_file_transfer_task_get_id(xfer_task), buffer, count); > > > > > > if (count == 0 || > > > spice_file_transfer_task_is_completed(xfer_task)) { > > > /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from > > > agent > > > > Interesting. I see now though that there's a corner case that could > > still be slightly problematic: > > > > For a zero-length file, we do a read and then this callback is > > triggered with count == 0. We send that (empty) message to the > > agent. > > But say the agent doesn't send back its SUCCESS status very quickly > > and > > so the _data_flushed_cb() function intiates another async read. > > Ah, but file_xfer_flush_async() is never called on zero-length files, > as > we have: > > if (count == 0 || spice_file_transfer_task_is_completed(xfer_task)) > { > /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent > * in case the task was completed, nothing to do. */ > return; > } > hmm, you're right. But it's a little bit weird that we don't flush this message but we do all other file-transfer messages. That means that for zero-length files we'll never receive GError reports for failed sending/flushing of the message. This doesn't matter at the moment since it doesn't look like the flush can actually fail. So I guess it doesn't really matter. > > > > When > > the async read callback is triggered again, we get another zero- > > length > > read. Since the file size is zero, we pass the above conditions and > > send out a second zero-length data message to the agent. That's the > > same problem you're trying to fix above for non-zero-length files. > > Yeah, it would be. I tried tranfering 10+ zero-length files to > windows > and linux guests to see if something weird would be seen but no. > > > > > It's > > clearly not as important because it's not at all common to transfer > > zero-length files, but it's probably still good to handle it. > > > > Another semi-related issue: what do we do if a buggy agent never > > sends > > a status response? Should we set a timer to cancel the transfer > > within > > a certain time after we've sent our last data message if we haven't > > received a response from the agent yet? Obviously that's not a > > issue to > > be solved in this patch, but it's somethign that I was wondering > > while > > reviewing the code. > > A timer is simple and would work. We could also clear some data after > sending all the payload as we will not use it anymore... and trigger > some warnings on termination if we never had the SUCCESS/ERROR > message > by them. > > > > > > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > Let me know if you think there is still an issue to be solved and > many > thanks for the review! Not really anything additional needed for this patch. It just triggered some thoughts about semi-related stuff. Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel