Bug 417645 - seekPos probably doesn't work at all
Summary: seekPos probably doesn't work at all
Alias: None
Product: kio-extras
Classification: Frameworks and Libraries
Component: SFTP (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Harald Sitter
Depends on:
Reported: 2020-02-14 15:40 UTC by Harald Sitter
Modified: 2020-03-25 12:35 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 20.04


Note You need to log in before you can comment on or make changes to this bug.
Description Harald Sitter 2020-02-14 15:40:50 UTC

`while ((offset = QT_LSEEK(fd, pos, mode)) == EAGAIN);`

QT_LSEEK is simply a define for lseek based on platform capabilities. lseek does return -1 on error + sets errno. otherwise it returns the offset. so, checking == EAGAIN makes absolutely no sense.

Test and probably fix seekPos (used for partial file resume).
Comment 1 Harald Sitter 2020-03-05 12:10:18 UTC
As expected the code is indeed wrong and loops indefinitely when a .part file is exactly EAGAIN (11). At least for remote->local scenarios it is also broken in actually resuming a part file -.-
Comment 2 Harald Sitter 2020-03-19 11:33:04 UTC
Git commit d8cf85ec2f0f13635de9482e8234f94f3bc444d1 by Harald Sitter.
Committed on 19/03/2020 at 11:31.
Pushed by sitter into branch 'master'.

sftp: fix seekPos + file resuming when part file is of size 11

previously seekPos would loop over offset==EAGAIN. the returned off_t of
seek is not an error, but the offset or -1. this incorrect handling
of the return value resulted in attempting to seek a file of the size 11
to get stuck in an infinite loop as EAGAIN==11 and so the loop would
always be true. any other file size would have been fine, so the impact
of this is actually super small.

also sync up the expectation and handling a bit more between copy and put
specifically we always seek to the size we (think) the part file has,
instead of letting the libc determine the end. this is in part so
we can simply do an offset==size comparison to check if the seek worked
to the end we expected it to.

the seekPos() helper was removed as it now serves no purpose over calling
lseek directly.
FIXED-IN: 20.04

Test Plan:
- create file
- `split -b 11` file to get a segment exactly EAGAIN size
- copy first segment to some other dir as file.part
- open sftp to other dir and copy file there
- copy doesn't get stuck, the file.part is removed, and the resulting file is same as input
- vice versa copy from sftp to local

Reviewers: ngraham, feverfew

Reviewed By: ngraham

Subscribers: bruns, kde-frameworks-devel, kfm-devel

Tags: #dolphin, #frameworks

Differential Revision: https://phabricator.kde.org/D27871

M  +6    -12   sftp/kio_sftp.cpp

Comment 3 Harald Sitter 2020-03-25 12:35:12 UTC
Git commit 8a04e10091308ad0642e6b164e6b19f640d9aeb4 by Harald Sitter.
Committed on 25/03/2020 at 12:34.
Pushed by sitter into branch 'master'.

sftp: fix partial transfer resuming when copying to local

the previous condition checked if the final target path size was >0,
which it would only be when the file already exists (i.e. overwrite) in
all other scenarios it would always be false and as such resuming wouldn't
work. what we actually want to check is whether the part file is >0 (i.e.
there's actual pending bytes to resume from).

this makes resuming work when copying remote->local

Test Plan:
- create file of suitably large size (1g)
- `split -b somesize` the file into two segments
- copy first segment to /tmp/file.part
- connect to /tmp over sftp and copy the file there
- progress starts at 50% and resulting file is same as input file

Reviewers: ngraham, feverfew, bruns

Subscribers: bruns, kde-frameworks-devel, kfm-devel

Tags: #dolphin, #frameworks

Differential Revision: https://phabricator.kde.org/D27872

M  +4    -2    sftp/kio_sftp.cpp