Bug 2108 - sftp progress meter updates too early
Summary: sftp progress meter updates too early
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sftp (show other bugs)
Version: 6.2p1
Hardware: Other Linux
: P5 enhancement
Assignee: Assigned to nobody
URL: http://bugs.debian.org/708372
Keywords:
Depends on:
Blocks: V_6_3
  Show dependency treegraph
 
Reported: 2013-05-25 08:03 AEST by Colin Watson
Modified: 2016-08-02 10:41 AEST (History)
1 user (show)

See Also:


Attachments
update sftp upload progress only when acks are received (1000 bytes, patch)
2013-05-25 08:03 AEST, Colin Watson
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Watson 2013-05-25 08:03:03 AEST
Created attachment 2275 [details]
update sftp upload progress only when acks are received

http://bugs.debian.org/708372 reports that the scp progress meter updates to 100% before the file has been fully transferred.  I looked at whether it's possible to fix this within the constraints of the existing scp "protocol" (such as it is), and I think it probably isn't; the scp client would have to know that it's expecting the scp target to send back acknowledgement messages, which is going to be pretty difficult to arrange.  So my first impulse was to reply to say that this isn't possible in scp, but that the reporter should use sftp instead, and perhaps some day we'll make the scp client actually use the SFTP protocol under the covers (as is done by e.g. PuTTY's pscp).  However, I then went to check that sftp behaves sensibly, and it has the exact same bug.  Oops.

Fortunately, I think this is easy to fix.  The sftp client already keeps a list of the acknowledgements it's waiting for so that it can block in a loop until they've all arrived, and those acknowledgements already record the length of each chunk of data.  So all we need to do is update the progress counter when acks come back rather than when we send the data, taking care to separate the progress counter from the offset of the next chunk of data to send.  How does this patch look?
Comment 1 Darren Tucker 2013-05-25 13:45:19 AEST
Comment on attachment 2275 [details]
update sftp upload progress only when acks are received

looks reasonable to me.  the packets are guaranteed to be processed in order for each file so it should even be accurate :-)
Comment 2 Colin Watson 2013-05-25 21:07:00 AEST
It doesn't even matter if they aren't processed in order, because all we're doing is adding the length from the ack to the progress counter, and that's commutative.
Comment 3 Darren Tucker 2013-06-02 08:35:49 AEST
Patch applied, thanks.
Comment 4 Damien Miller 2016-08-02 10:41:41 AEST
Close all resolved bugs after 7.3p1 release