openssh bugs - May 2013 - [Bug 2108] New: sftp progress meter updates too early

https://bugzilla.mindrot.org/show_bug.cgi?id=2108

            Bug ID: 2108
           Summary: sftp progress meter updates too early
           Product: Portable OpenSSH
           Version: 6.2p1
          Hardware: Other
               URL: http://bugs.debian.org/708372
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sftp
          Assignee: unassigned-bugs at mindrot.org
          Reporter: cjwatson at debian.org

Created attachment 2275
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2275&action=edit
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?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2108

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
   Attachment #2275|                            |ok+
              Flags|                            |

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2275
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2275
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 :-)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2108

--- Comment #2 from Colin Watson <cjwatson at debian.org> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2108

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2076

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2108

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
Patch applied, thanks.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2108

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.