Fix transfer only partially transferring #3902
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3902 +/- ##
==========================================
- Coverage 89.11% 89.11% -0.01%
==========================================
Files 255 255
Lines 14600 14602 +2
==========================================
+ Hits 13011 13012 +1
- Misses 1589 1590 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @Sploder12, good work. A couple of matters for discussion inline.
src/ssh/sftp_client.cpp
Outdated
| @@ -331,7 +330,8 @@ void SFTPClient::do_push_file(std::istream& source, const fs::path& target_path) | |||
| if (!remote_file) | |||
| throw SFTPError{"cannot open remote file {}: {}", target_path, ssh_get_error(sftp->session)}; | |||
|
|
|||
| std::array<char, max_transfer> buffer{}; | |||
| const auto max_write = sftp_limits(sftp.get())->max_write_length; | |||
| std::vector<char> buffer(max_write); | |||
There was a problem hiding this comment.
We shouldn't need to zero all the bytes, right? I wonder if we should do std::vector<char> buffer; buffer.reserve(max_write); instead, here and in the read case below (perhaps with a warning comment about the uninitialized data).
Also, this is a tricky constructor: if someone accidentally replaced the parenthesis with curly braces, it would create a vector with a single character. If you think we'd better keep, do you think it's worth a comment?
There was a problem hiding this comment.
I think that'd be undefined behavior. But std::unique_ptr<char[]> buffer{new char[max_write]}; should work.
tests/test_sftp_client.cpp
Outdated
| REPLACE(sftp_init, [this](sftp_session sftp) { | ||
| sftp->limits = &limits; | ||
| return SSH_OK; | ||
| }); |
There was a problem hiding this comment.
This code is repeated many times. Would you mind extracting this into a separate function to call REPLACE with?
Fixes the issue of
multipass transferonly transferring some chunks of the file. This usessftp_limitsinstead of the constant65536uto determine the correct buffer size for SFTP reads and writes.Fixes #3892
MULTI-1768