Skip to content
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 silently ignoring STATUS response after READ #240

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gralpli
Copy link

@gralpli gralpli commented Jan 9, 2021

This attempts to fix #224, but is incomplete yet. Open questions:

  • Does res need to be set in the immediately following else-if-branch and in the last else-branch as well? I guess not, because the case when chunk->sio.error equals MY_EOF is explicitly guarded a couple lines above.

  • Do I need such a MY_EOF guard for the line that I added?

  • When encountering an I/O error, SFTP reports a SSH_FX_FAILURE which, by the function sftp_error_to_errno, is mapped to an EPERM. This leads to us getting an “Operation not permitted” over sshfs instead of an I/O error. Can EPERM be replaced by EIO?

My addition of the word else is purely for stylistic reasons and does not change the control flow.

@Nikratio
Copy link
Contributor

Thanks for the patch! I'm afraid I do not know the answers to your questions. The best I can do is to study the code carefully myself and try to figure this out, so this review might take a while. Any help is appreciated.

Once thing that comes to mind is that we probably need to check how real permissions errors are reported - we wouldn't want to get EIO if permissions are incorrect.

@Nikratio
Copy link
Contributor

Is this ready for review? I understand from the original message that there are still open questions, but it's not clear to me if you're still working on them (or other issues) or if you're waiting for someone to review this.

@gralpli
Copy link
Author

gralpli commented Apr 1, 2022

This is my first pull request where I’m actually fixing something and I don’t know how confident I need to be to release it. As far as I remember, this fixes the bug. But I can’t estimate all the implications that this code change has.

So I’d say yes, this is ready for review. The open questions are questions that a reviewer would need to answer to check if the code is correct.

@Nikratio
Copy link
Contributor

Nikratio commented Apr 1, 2022

First step is to make sure the tests in the CI still pass, currently they are failing. That means either your code has a bug or the test has a bug - but in both cases it needs to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read error on remote is reported as EOF
2 participants