D27872: sftp: fix partial transfer resuming when copying to local
This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit R320:8a04e1009130: sftp: fix partial transfer resuming when copying to local (authored by sitter). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D27872?vs=77076=78445#toc REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27872?vs=77076=78445 REVISION DETAIL https://phabricator.kde.org/D27872 AFFECTED FILES sftp/kio_sftp.cpp To: sitter, ngraham, feverfew, bruns Cc: bruns, kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
D27872: sftp: fix partial transfer resuming when copying to local
sitter added a comment. ping REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D27872 To: sitter, ngraham, feverfew, bruns Cc: bruns, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
D27872: sftp: fix partial transfer resuming when copying to local
sitter added inline comments. INLINE COMMENTS > bruns wrote in kio_sftp.cpp:1935 > Dependent on the file system, this will no longer catch `partFile.isDir()`. > stat.st_size is only defined for regular files and symlinks, for all other > types it is implementation defined. E.g. XFS and Btrfs return 0 for empty > directories. Your comment is dazzling. What you meant to say is that the size check ought to be after the dir check? > bruns wrote in kio_sftp.cpp:1936 > should be `if (!partFile.isFile())` - we cant resume a pipe or socket ... Please make a diff. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D27872 To: sitter, ngraham, feverfew, bruns Cc: bruns, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
D27872: sftp: fix partial transfer resuming when copying to local
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kio_sftp.cpp:1935
>
> -if (bMarkPartial && bPartExists && copyFile.size() > 0) {
> +if (bMarkPartial && bPartExists && partFile.size() > 0) {
> if (partFile.isDir()) {
Dependent on the file system, this will no longer catch `partFile.isDir()`.
stat.st_size is only defined for regular files and symlinks, for all other
types it is implementation defined. E.g. XFS and Btrfs return 0 for empty
directories.
> kio_sftp.cpp:1936
> +if (bMarkPartial && bPartExists && partFile.size() > 0) {
> if (partFile.isDir()) {
> return Result::fail(ERR_FILE_ALREADY_EXIST, sCopyFile);
should be `if (!partFile.isFile())` - we cant resume a pipe or socket ...
REPOSITORY
R320 KIO Extras
REVISION DETAIL
https://phabricator.kde.org/D27872
To: sitter, ngraham, feverfew, bruns
Cc: bruns, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice,
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew,
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros,
emmanuelp, mikesomov
D27872: sftp: fix partial transfer resuming when copying to local
sitter updated this revision to Diff 77076. sitter added a comment. . REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27872?vs=77037=77076 BRANCH sftp-resume-to-local REVISION DETAIL https://phabricator.kde.org/D27872 AFFECTED FILES sftp/kio_sftp.cpp To: sitter, ngraham, feverfew, bruns Cc: bruns, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
D27872: sftp: fix partial transfer resuming when copying to local
bruns requested changes to this revision. bruns added a comment. This revision now requires changes to proceed. Erroneous submit? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D27872 To: sitter, ngraham, feverfew, bruns Cc: bruns, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
D27872: sftp: fix partial transfer resuming when copying to local
sitter created this revision. sitter added reviewers: ngraham, feverfew. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. sitter requested review of this revision. REVISION SUMMARY 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 CCBUG: 417645 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 REPOSITORY R320 KIO Extras BRANCH sftp-resume-to-local REVISION DETAIL https://phabricator.kde.org/D27872 AFFECTED FILES sftp/kio_sftp.cpp To: sitter, ngraham, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
