-
-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix reading large files over SCP #621
Conversation
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
@Jakuje FYI the linting failures block this PR |
The suggestions accepted, the linting should pass now. |
@Jakuje hey, so I added a few improvements on top and posted a few more comments inline. I've now stopped pushing to the PR branch so feel free to clean it up/rebase/make more changes. |
This needs conflict resolution now. |
Signed-off-by: Jakub Jelen <[email protected]> Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Signed-off-by: Jakub Jelen <[email protected]>
Signed-off-by: Jakub Jelen <[email protected]>
Signed-off-by: Jakub Jelen <[email protected]>
Resolved the conflict and dropped the check for too large replies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Looks like one of the CI jobs got stuck with #557 so I restarted it: https://github.com/ansible/pylibssh/actions/runs/9699687288/job/26769948869?pr=621#step:15:152. |
@Jakuje this has been released as a part of v1.2.2: https://github.com/ansible/pylibssh/releases/tag/v1.2.2 / #626 / https://pypi.org/project/ansible-pylibssh/1.2.2/. Enjoy! 🎉 |
SUMMARY
The current SCP code does not handle large reads. Instead just allocates the large buffer fills up to 64k bytes and then writes the whole to the file (most likely the rest filled with junk).
The libssh returns the number of read bytes from
ssh_scp_read()
and for practical reasons does not allocate the whole file in memory. The reads are capped to 64k B.This PR fixes the read by both not allocating the entire file size in memory and reading it by chunks (that much max libssh chunk atm) and writing that to the local file.
ISSUE TYPE
ADDITIONAL INFORMATION
The attached test case is failing without the change, with errors like this:
(lines do not match)
Additionally, there is one more test case, that I suspected will be problematic, but it turned out it works just ok, so leaving for the coverage.